From 5fa0931a89ff6bfd0efad96ca97b565fcfb59ff4 Mon Sep 17 00:00:00 2001 From: James Turner Date: Thu, 13 Feb 2014 12:36:06 +0000 Subject: [PATCH] Revised set_bucket implementation. - sink some inline methods into the .cxx file - add isValid test to SGBucket - sgGetBuckets won't return invalid buckets near the poles - additional unit tests --- simgear/bucket/newbucket.cxx | 180 ++++++++++++++++++++------------- simgear/bucket/newbucket.hxx | 46 +++------ simgear/bucket/test_bucket.cxx | 51 ++++++++-- 3 files changed, 165 insertions(+), 112 deletions(-) diff --git a/simgear/bucket/newbucket.cxx b/simgear/bucket/newbucket.cxx index c8d2cd02..245b6f14 100644 --- a/simgear/bucket/newbucket.cxx +++ b/simgear/bucket/newbucket.cxx @@ -27,17 +27,41 @@ # include #endif -#include +#include +#include #include +#include #include "newbucket.hxx" // default constructor -SGBucket::SGBucket() { +SGBucket::SGBucket() : + lon(-1000), + lat(-1000), + x(0), + y(0) +{ } +bool SGBucket::isValid() const +{ + // The most northerly valid latitude is 89, not 90. There is no tile + // whose *bottom* latitude is 90. Similar there is no tile whose left egde + // is 180 longitude. + return (lon >= -180) && + (lon < 180) && + (lat >= -90) && + (lat < 90) && + (x < 8) && (y < 8); +} + +void SGBucket::make_bad() +{ + lon = -1000; + lat = -1000; +} // constructor for specified location SGBucket::SGBucket(const double dlon, const double dlat) { @@ -45,18 +69,10 @@ SGBucket::SGBucket(const double dlon, const double dlat) { } SGBucket::SGBucket(const SGGeod& geod) { - set_bucket(geod); + set_bucket(geod.getLongitudeDeg(), + geod.getLatitudeDeg()); } -// create an impossible bucket if false -SGBucket::SGBucket(const bool is_good) { - set_bucket(0.0, 0.0); - if ( !is_good ) { - lon = -1000; - } -} - - // Parse a unique scenery tile index and find the lon, lat, x, and y SGBucket::SGBucket(const long int bindex) { long int index = bindex; @@ -75,42 +91,49 @@ SGBucket::SGBucket(const long int bindex) { x = index; } +/* Calculate the greatest integral value less than + * or equal to the given value (floor(x)), + * but attribute coordinates close to the boundary to the next + * (increasing) integral + */ +static int floorWithEpsilon(double x) +{ + double diff = x - static_cast(x); + if ( (x >= 0.0) || (fabs(diff) < SG_EPSILON) ) { + return static_cast(x); + } else { + return static_cast(x) - 1; + } +} // Set the bucket params for the specified lat and lon -void SGBucket::set_bucket( double dlon, double dlat ) { +void SGBucket::set_bucket( double dlon, double dlat ) +{ + if ((dlon < -180.0) || (dlon >= 180.0)) { + SG_LOG(SG_TERRAIN, SG_WARN, "SGBucket::set_bucket: passed longitude:" << dlon); + dlon = SGMiscd::normalizePeriodic(-180.0, 180.0, dlon); + } + + if ((dlat < -90.0) || (dlat > 90.0)) { + SG_LOG(SG_TERRAIN, SG_WARN, "SGBucket::set_bucket: passed latitude" << dlat); + dlat = SGMiscd::clip(dlat, -90.0, 90.0); + } + // - // latitude first + // longitude first // double span = sg_bucket_span( dlat ); - double diff = dlon - (double)(int)dlon; - - // cout << "diff = " << diff << " span = " << span << endl; - - /* Calculate the greatest integral longitude less than - * or equal to the given longitude (floor(dlon)), - * but attribute coordinates near the east border - * to the next tile. - */ - if ( (dlon >= 0) || (fabs(diff) < SG_EPSILON) ) { - lon = (int)dlon; - } else { - lon = (int)dlon - 1; - } - + // we do NOT need to special case lon=180 here, since + // normalizePeriodic will never return 180; it will + // return -180, which is what we want. + lon = floorWithEpsilon(dlon); + // find subdivision or super lon if needed - if ( span < SG_EPSILON ) { - /* sg_bucket_span() never returns 0.0 - * or anything near it, so this really - * should not occur at any time. - */ - // polar cap - lon = 0; - x = 0; - } else if ( span <= 1.0 ) { + if ( span <= 1.0 ) { /* We have more than one tile per degree of * longitude, so we need an x offset. */ - x = (int)((dlon - lon) / span); + x = (int)((dlon - lon) / span); } else { /* We have one or more degrees per tile, * so we need to find the base longitude @@ -123,45 +146,30 @@ void SGBucket::set_bucket( double dlon, double dlat ) { * * That way, the Greenwich Meridian is always * a tile border. - * - * This gets us into trouble with the polar caps, - * which have width 360 and thus either span - * the range from 0 to 360 or from -360 to 0 - * degrees, depending on whether lon is positive - * or negative! - * - * We also get into trouble with the 8 degree tiles - * north of 88N and south of 88S, because the west- - * and east-most tiles in that range will cover 184W - * to 176W and 176E to 184E respectively, with their - * center at 180E/W! */ - lon=(int)floor(floor((lon+SG_EPSILON)/span)*span); - /* Correct the polar cap issue */ - if ( lon < -180 ) { - lon = -180; - } - x = 0; + lon=static_cast(floor(lon / span) * span); + x = 0; } // // then latitude // - diff = dlat - (double)(int)dlat; - - /* Again, a modified floor() function (see longitude) */ - if ( (dlat >= 0) || (fabs(diff) < SG_EPSILON) ) { - lat = (int)dlat; + lat = floorWithEpsilon(dlat); + + // special case when passing in the north pole point (possibly due to + // clipping latitude above). Ensures we generate a valid bucket in this + // scenario + if (lat == 90) { + lat = 89; + y = 7; } else { - lat = (int)dlat - 1; + /* Latitude base and offset are easier, as + * tiles always are 1/8 degree of latitude wide. + */ + y = (int)((dlat - lat) * 8); } - /* Latitude base and offset are easier, as - * tiles always are 1/8 degree of latitude wide. - */ - y = (int)((dlat - lat) * 8); } - void SGBucket::set_bucket(const SGGeod& geod) { set_bucket(geod.getLongitudeDeg(), geod.getLatitudeDeg()); @@ -256,7 +264,18 @@ double SGBucket::get_height_m() const { SGBucket SGBucket::sibling(int dx, int dy) const { + if (!isValid()) { + SG_LOG(SG_TERRAIN, SG_WARN, "SGBucket::sibling: requesting sibling of invalid bucket"); + return SGBucket(); + } + double clat = get_center_lat() + dy * SG_BUCKET_SPAN; + // return invalid here instead of clipping, so callers can discard + // invalid buckets without having to check if it's an existing one + if ((clat < -90.0) || (clat > 90.0)) { + return SGBucket(); + } + // find the lon span for the new latitude double span = sg_bucket_span( clat ); @@ -265,6 +284,15 @@ SGBucket SGBucket::sibling(int dx, int dy) const return SGBucket(tmp, clat); } +std::string SGBucket::gen_index_str() const +{ + char tmp[20]; + std::snprintf(tmp, 20, "%ld", + (((long)lon + 180) << 14) + ((lat + 90) << 6) + + (y << 3) + x); + return (std::string)tmp; +} + // find the bucket which is offset by the specified tile units in the // X & Y direction. We need the current lon and lat to resolve // ambiguities when going from a wider tile to a narrower one above or @@ -354,8 +382,20 @@ void sgGetBuckets( const SGGeod& min, const SGGeod& max, std::vector& for (lat = min.getLatitudeDeg(); lat <= max.getLatitudeDeg(); lat += SG_BUCKET_SPAN) { span = sg_bucket_span( lat ); - for (lon = min.getLongitudeDeg(); lon <= max.getLongitudeDeg(); lon += span) { - list.push_back( SGBucket(lon , lat) ); + for (lon = min.getLongitudeDeg(); lon <= max.getLongitudeDeg(); lon += span) + { + SGBucket b(lon, lat); + if (!b.isValid()) { + continue; + } + + list.push_back(b); } } } + +std::ostream& operator<< ( std::ostream& out, const SGBucket& b ) +{ + return out << b.lon << ":" << (int)b.x << ", " << b.lat << ":" << (int)b.y; +} + diff --git a/simgear/bucket/newbucket.hxx b/simgear/bucket/newbucket.hxx index 96495aba..b891ef39 100644 --- a/simgear/bucket/newbucket.hxx +++ b/simgear/bucket/newbucket.hxx @@ -39,9 +39,8 @@ #include #include -#include // sprintf() -#include #include +#include #include /** @@ -99,16 +98,21 @@ class SGBucket { private: short lon; // longitude index (-180 to 179) short lat; // latitude index (-90 to 89) - char x; // x subdivision (0 to 7) - char y; // y subdivision (0 to 7) + unsigned char x; // x subdivision (0 to 7) + unsigned char y; // y subdivision (0 to 7) public: /** - * Default constructor. + * Default constructor, creates an invalid SGBucket */ SGBucket(); + /** + * Check if this bucket refers to a valid tile, or not. + */ + bool isValid() const; + /** * Construct a bucket given a specific location. * @param dlon longitude specified in degrees @@ -123,14 +127,6 @@ public: */ SGBucket(const SGGeod& geod); - /** Construct a bucket. - * @param is_good if false, create an invalid bucket. This is - * useful * if you are comparing cur_bucket to last_bucket and - * you want to * make sure last_bucket starts out as something - * impossible. - */ - SGBucket(const bool is_good); - /** Construct a bucket given a unique bucket index number. * @param bindex unique bucket index */ @@ -156,11 +152,8 @@ public: * and you want to make sure last_bucket starts out as something * impossible. */ - inline void make_bad() { - set_bucket(0.0, 0.0); - lon = -1000; - } - + void make_bad(); + /** * Generate the unique scenery tile index for this bucket * @@ -185,14 +178,8 @@ public: * string form. * @return tile index in string form */ - inline std::string gen_index_str() const { - char tmp[20]; - std::sprintf(tmp, "%ld", - (((long)lon + 180) << 14) + ((lat + 90) << 6) - + (y << 3) + x); - return (std::string)tmp; - } - + std::string gen_index_str() const; + /** * Build the base path name for this bucket. * @return base path in string form @@ -335,12 +322,7 @@ void sgGetBuckets( const SGGeod& min, const SGGeod& max, std::vector& * @param out output stream * @param b bucket */ -inline std::ostream& -operator<< ( std::ostream& out, const SGBucket& b ) -{ - return out << b.lon << ":" << (int)b.x << ", " << b.lat << ":" << (int)b.y; -} - +std::ostream& operator<< ( std::ostream& out, const SGBucket& b ); /** * Compare two bucket structures for equality. diff --git a/simgear/bucket/test_bucket.cxx b/simgear/bucket/test_bucket.cxx index d05f33c5..481f97e2 100644 --- a/simgear/bucket/test_bucket.cxx +++ b/simgear/bucket/test_bucket.cxx @@ -52,6 +52,7 @@ void testBasic() COMPARE(b1.get_y(), 0); COMPARE(b1.gen_index(), 3040320); COMPARE(b1.gen_base_path(), "e000n50/e005n55"); + VERIFY(b1.isValid()); SGBucket b2(-10.1, -43.8); COMPARE(b2.get_chunk_lon(), -11); @@ -59,6 +60,7 @@ void testBasic() COMPARE(b2.get_x(), 3); COMPARE(b2.get_y(), 1); // latitude chunks numbered bottom to top, it seems COMPARE(b2.gen_base_path(), "w020s50/w011s44"); + VERIFY(b2.isValid()); SGBucket b3(123.48, 9.01); COMPARE(b3.get_chunk_lon(), 123); @@ -66,7 +68,37 @@ void testBasic() COMPARE(b3.get_x(), 3); COMPARE(b3.get_y(), 0); COMPARE(b3.gen_base_path(), "e120n00/e123n09"); + VERIFY(b3.isValid()); + + SGBucket defBuck; + VERIFY(!defBuck.isValid()); + + b3.make_bad(); + VERIFY(!b3.isValid()); + SGBucket atAntiMeridian(180.0, 12.3); + VERIFY(atAntiMeridian.isValid()); + COMPARE(atAntiMeridian.get_chunk_lon(), -180); + COMPARE(atAntiMeridian.get_x(), 0); + + SGBucket atAntiMeridian2(-180.0, -78.1); + VERIFY(atAntiMeridian2.isValid()); + COMPARE(atAntiMeridian2.get_chunk_lon(), -180); + COMPARE(atAntiMeridian2.get_x(), 0); + +// check comparisom operator overload + SGBucket b4(5.11, 55.1); + VERIFY(b1 == b4); // should be equal + VERIFY(b1 == b1); + VERIFY(b1 != defBuck); + VERIFY(b1 != b2); + +// check wrapping/clipping of inputs + SGBucket wrapMeridian(-200.0, 45.0); + COMPARE(wrapMeridian.get_chunk_lon(), 160); + + SGBucket clipPole(48.9, 91); + COMPARE(clipPole.get_chunk_lat(), 89); } void testPolar() @@ -78,6 +110,11 @@ void testPolar() COMPARE(b1.get_x(), 0); COMPARE(b1.get_y(), 7); + COMPARE(b2.get_chunk_lat(), 89); + COMPARE(b2.get_chunk_lon(), 0); + COMPARE(b2.get_x(), 0); + COMPARE(b2.get_y(), 7); + COMPARE(b1.gen_index(), b2.gen_index()); SGGeod actualNorthPole1 = b1.get_corner(2); @@ -108,9 +145,8 @@ void testPolar() COMPARE_EP(actualSouthPole2.getLatitudeDeg(), -90.0); COMPARE_EP(actualSouthPole2.getLongitudeDeg(), -168); - // no automatic wrapping of these values occurs SGBucket b7(200, 89.88); - COMPARE(b7.get_chunk_lon(), 192); + COMPARE(b7.get_chunk_lon(), -168); } @@ -198,16 +234,11 @@ void testPolarOffset() COMPARE(b6.gen_index(), sgBucketOffset(177, 87.3, 1, 1)); -#if 0 // offset vertically towards the pole - SGBucket b3(b1.sibling(0, -5)); - COMPARE(b3.get_chunk_lat(), -90); - COMPARE(b3.get_chunk_lon(), -12); - COMPARE(b3.get_x(), 0); - COMPARE(b3.get_y(), 0); + SGBucket b7(b1.sibling(0, -5)); + VERIFY(!b7.isValid()); - COMPARE(b3.gen_index(), sgBucketOffset(-11.7, -89.6, 0, 0)); -#endif + VERIFY(!SGBucket(0, 90).sibling(0, 1).isValid()); } // test behaviour of bucket-offset near the anti-meridian (180-meridian)