Geo: fix computation of bounding box.

A bug was reported in the context in issue #3631. The root cause of the
bug was that certain neighbor boxes were zeroed after the "inside the
bounding box or not" check, simply because the bounding box computation
function was wrong.

A few debugging infos where enhanced and moved in other parts of the
code. A check to avoid steps=0 was added, but is unrelated to this
issue and I did not verified it was an actual bug in practice.
This commit is contained in:
antirez 2016-12-05 14:02:32 +01:00
parent 434e6b2da3
commit 001138aec3
2 changed files with 33 additions and 44 deletions

View File

@ -326,6 +326,7 @@ int membersOfGeoHashBox(robj *zobj, GeoHashBits hash, geoArray *ga, double lon,
int membersOfAllNeighbors(robj *zobj, GeoHashRadius n, double lon, double lat, double radius, geoArray *ga) { int membersOfAllNeighbors(robj *zobj, GeoHashRadius n, double lon, double lat, double radius, geoArray *ga) {
GeoHashBits neighbors[9]; GeoHashBits neighbors[9];
unsigned int i, count = 0, last_processed = 0; unsigned int i, count = 0, last_processed = 0;
int debugmsg = 0;
neighbors[0] = n.hash; neighbors[0] = n.hash;
neighbors[1] = n.neighbors.north; neighbors[1] = n.neighbors.north;
@ -340,8 +341,26 @@ int membersOfAllNeighbors(robj *zobj, GeoHashRadius n, double lon, double lat, d
/* For each neighbor (*and* our own hashbox), get all the matching /* For each neighbor (*and* our own hashbox), get all the matching
* members and add them to the potential result list. */ * members and add them to the potential result list. */
for (i = 0; i < sizeof(neighbors) / sizeof(*neighbors); i++) { for (i = 0; i < sizeof(neighbors) / sizeof(*neighbors); i++) {
if (HASHISZERO(neighbors[i])) if (HASHISZERO(neighbors[i])) {
if (debugmsg) D("neighbors[%d] is zero",i);
continue; continue;
}
/* Debugging info. */
if (debugmsg) {
GeoHashRange long_range, lat_range;
geohashGetCoordRange(&long_range,&lat_range);
GeoHashArea myarea = {{0}};
geohashDecode(long_range, lat_range, neighbors[i], &myarea);
/* Dump center square. */
D("neighbors[%d]:\n",i);
D("area.longitude.min: %f\n", myarea.longitude.min);
D("area.longitude.max: %f\n", myarea.longitude.max);
D("area.latitude.min: %f\n", myarea.latitude.min);
D("area.latitude.max: %f\n", myarea.latitude.max);
D("\n");
}
/* When a huge Radius (in the 5000 km range or more) is used, /* When a huge Radius (in the 5000 km range or more) is used,
* adjacent neighbors can be the same, leading to duplicated * adjacent neighbors can be the same, leading to duplicated
@ -350,7 +369,11 @@ int membersOfAllNeighbors(robj *zobj, GeoHashRadius n, double lon, double lat, d
if (last_processed && if (last_processed &&
neighbors[i].bits == neighbors[last_processed].bits && neighbors[i].bits == neighbors[last_processed].bits &&
neighbors[i].step == neighbors[last_processed].step) neighbors[i].step == neighbors[last_processed].step)
{
if (debugmsg)
D("Skipping processing of %d, same as previous\n",i);
continue; continue;
}
count += membersOfGeoHashBox(zobj, neighbors[i], ga, lon, lat, radius); count += membersOfGeoHashBox(zobj, neighbors[i], ga, lon, lat, radius);
last_processed = i; last_processed = i;
} }

View File

@ -82,30 +82,18 @@ uint8_t geohashEstimateStepsByRadius(double range_meters, double lat) {
return step; return step;
} }
/* Return the bounding box of the search area centered at latitude,longitude
* having a radius of radius_meter. bounds[0] - bounds[2] is the minimum
* and maxium longitude, while bounds[1] - bounds[3] is the minimum and
* maximum latitude. */
int geohashBoundingBox(double longitude, double latitude, double radius_meters, int geohashBoundingBox(double longitude, double latitude, double radius_meters,
double *bounds) { double *bounds) {
if (!bounds) return 0; if (!bounds) return 0;
double lonr, latr; bounds[0] = longitude - rad_deg(radius_meters/EARTH_RADIUS_IN_METERS/cos(deg_rad(latitude)));
lonr = deg_rad(longitude); bounds[2] = longitude + rad_deg(radius_meters/EARTH_RADIUS_IN_METERS/cos(deg_rad(latitude)));
latr = deg_rad(latitude); bounds[1] = latitude - rad_deg(radius_meters/EARTH_RADIUS_IN_METERS);
bounds[3] = latitude + rad_deg(radius_meters/EARTH_RADIUS_IN_METERS);
if (radius_meters > EARTH_RADIUS_IN_METERS)
radius_meters = EARTH_RADIUS_IN_METERS;
double distance = radius_meters / EARTH_RADIUS_IN_METERS;
double min_latitude = latr - distance;
double max_latitude = latr + distance;
/* Note: we're being lazy and not accounting for coordinates near poles */
double min_longitude, max_longitude;
double difference_longitude = asin(sin(distance) / cos(latr));
min_longitude = lonr - difference_longitude;
max_longitude = lonr + difference_longitude;
bounds[0] = rad_deg(min_longitude);
bounds[1] = rad_deg(min_latitude);
bounds[2] = rad_deg(max_longitude);
bounds[3] = rad_deg(max_latitude);
return 1; return 1;
} }
@ -158,35 +146,13 @@ GeoHashRadius geohashGetAreasByRadius(double longitude, double latitude, double
< radius_meters) decrease_step = 1; < radius_meters) decrease_step = 1;
} }
if (decrease_step) { if (steps > 1 && decrease_step) {
steps--; steps--;
geohashEncode(&long_range,&lat_range,longitude,latitude,steps,&hash); geohashEncode(&long_range,&lat_range,longitude,latitude,steps,&hash);
geohashNeighbors(&hash,&neighbors); geohashNeighbors(&hash,&neighbors);
geohashDecode(long_range,lat_range,hash,&area); geohashDecode(long_range,lat_range,hash,&area);
} }
/* Example debug info. This turns to be very useful every time there is
* to investigate radius search potential bugs. So better to leave it
* here. */
if (0) {
GeoHashArea myarea = {{0}};
geohashDecode(long_range, lat_range, neighbors.west, &myarea);
/* Dump West. */
D("Neighbors");
D("area.longitude.min: %f\n", myarea.longitude.min);
D("area.longitude.max: %f\n", myarea.longitude.max);
D("area.latitude.min: %f\n", myarea.latitude.min);
D("area.latitude.max: %f\n", myarea.latitude.max);
/* Dump center square. */
D("Area");
D("area.longitude.min: %f\n", area.longitude.min);
D("area.longitude.max: %f\n", area.longitude.max);
D("area.latitude.min: %f\n", area.latitude.min);
D("area.latitude.max: %f\n", area.latitude.max);
}
/* Exclude the search areas that are useless. */ /* Exclude the search areas that are useless. */
if (area.latitude.min < min_lat) { if (area.latitude.min < min_lat) {
GZERO(neighbors.south); GZERO(neighbors.south);