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

@@ -27,17 +27,41 @@
 #  include <simgear_config.h>
 #endif
 
-#include <math.h>
+#include <cmath>
+#include <iostream>
 
 #include <simgear/misc/sg_path.hxx>
+#include <simgear/debug/logstream.hxx>
 
 #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<int>(x);
+    if ( (x >= 0.0) || (fabs(diff) < SG_EPSILON) ) {
+        return static_cast<int>(x);
+    } else {
+        return static_cast<int>(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;
+    // we do NOT need to special case lon=180 here, since
-
+    // normalizePeriodic will never return 180; it will
-    // cout << "diff = " << diff << "  span = " << span << endl;
+    // return -180, which is what we want.
-
+    lon = floorWithEpsilon(dlon);
-    /* 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;
-    }
-
     // find subdivision or super lon if needed
-    if ( span < SG_EPSILON ) {
+    if ( span <= 1.0 ) {
-        /* 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 ) {
         /* 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);
+        lon=static_cast<int>(floor(lon / span) * span);
-        /* Correct the polar cap issue */
+        x = 0;
-        if ( lon < -180 ) {
-            lon = -180;
-        }
-	x = 0;
     }
 
     //
     // then latitude
     //
-    diff = dlat - (double)(int)dlat;
+    lat = floorWithEpsilon(dlat);
-
+    
-    /* Again, a modified floor() function (see longitude) */
+    // special case when passing in the north pole point (possibly due to
-    if ( (dlat >= 0) || (fabs(diff) < SG_EPSILON) ) {
+    // clipping latitude above). Ensures we generate a valid bucket in this
-	lat = (int)dlat;
+    // 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<SGBucket>&
 
     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) {
+        for (lon = min.getLongitudeDeg(); lon <= max.getLongitudeDeg(); lon += span)
-            list.push_back( SGBucket(lon , lat) );
+        {
+            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;
+}
+

simgear/bucket/newbucket.hxx

@@ -39,9 +39,8 @@
 #include <simgear/math/SGMath.hxx>
 
 #include <cmath>
-#include <cstdio> // sprintf()
-#include <ostream>
 #include <string>
+#include <iosfwd>
 #include <vector>
 
 /**
@@ -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)
+    unsigned char x;          // x subdivision (0 to 7)
-    char y;          // y 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() {
+    void make_bad();
-	set_bucket(0.0, 0.0);
+    
-	lon = -1000;
-    }
-
     /**
      * 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 {
+    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;
-    }
-
     /**
      * 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<SGBucket>&
  * @param out output stream
  * @param b bucket
  */
-inline std::ostream&
+std::ostream& operator<< ( std::ostream& out, const SGBucket& b );
-operator<< ( std::ostream& out, const SGBucket& b )
-{
-    return out << b.lon << ":" << (int)b.x << ", " << b.lat << ":" << (int)b.y;
-}
-
 
 /**
  * Compare two bucket structures for equality.

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));
+    SGBucket b7(b1.sibling(0, -5));
-    COMPARE(b3.get_chunk_lat(), -90);
+    VERIFY(!b7.isValid());
-    COMPARE(b3.get_chunk_lon(), -12);
-    COMPARE(b3.get_x(), 0);
-    COMPARE(b3.get_y(), 0);
     
-    COMPARE(b3.gen_index(), sgBucketOffset(-11.7, -89.6, 0, 0));
+    VERIFY(!SGBucket(0, 90).sibling(0, 1).isValid());
-#endif
 }
 
 // test behaviour of bucket-offset near the anti-meridian (180-meridian)