Fix kml bounding box issues. #761
Conversation
The kml bouning box had problems around the antimeridian, that were
unsuccessfully mitigated in 2011. Instead of fixing the obvious
mistake in that mitigation we use nvectors to compute the actual
center of the bounding box.
Nvectors will be useful to avoid problems at discontinuities and
singularites that are inherent in the latitude,longitude coordinate
system.
| return result; | ||
| } | ||
|
|
||
| #if 0 |
There was a problem hiding this comment.
See ifi the four 'if 0' blocks in this file really do deserve to stay.
| // equation 10 (normalized, n_EA_E and kaeast are perpendicular unit vectors) | ||
| Vector3D kanorth = crossProduct(n_EA_E, kaeast); | ||
|
|
||
| Vector3D d_EA_E = kanorth*cos(az_rad) + kaeast*sin(az_rad); |
There was a problem hiding this comment.
Please run astyle or clang-tidy to format. This is one of many places that binary operand spacing is inconsistent. Here it's both wrong (*) and right (+) in the same line.
There was a problem hiding this comment.
The only thing astyle changes with our style file seems incorrect, it eliminates the space before the '*' in
WGS84_ECCENTRICITY_SQUARED = 1.0 - (WGS84_ASPECT_RATIO * WGS84_ASPECT_RATIO)
I don't want to get into formatting flip flops with two rulers, i.e. astyle and clang-format.
There was a problem hiding this comment.
astyle could do what I think you want with "--pad-oper", but that hasn't been part of our style file.
| public: | ||
| LatLon(double latitude, double longitude); | ||
|
|
||
| double lat; |
There was a problem hiding this comment.
Can class members be foo_ instead of foo? Or is that too mucky given how prevalent thinks like this and vec3d are in our code.
There was a problem hiding this comment.
The LatLon member variables are public.
I actually tried to change the Vector3D fields as you suggest but I got into a battle with clion I didn't want to fight. I could try again. I note our getters for that class violate https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c131-avoid-trivial-getters-and-setters, and if we wanted to heed that guideline then the trailing underscore is undesirable. But, at this point, the fields are not public and could use a trailing underscore.
There was a problem hiding this comment.
I renamed the member variables of Vector3D as you suggested.
| @@ -8,10 +8,10 @@ | |||
| <end>2010-05-28T02:41:44Z</end> | |||
| </gx:TimeSpan> | |||
| <longitude>-122.139608</longitude> | |||
| <latitude>37.382794</latitude> | |||
| <latitude>37.382814</latitude> | |||
There was a problem hiding this comment.
@viettaml , note this CL is going to change the LookAt (and thus, FlyTo) on files we handle. It's small on most common data and cases involving > hemisphere of polygons or places spanning hemispheres are subject to weird stuff already.
We can at least reduce OUR weird stuff.
There was a problem hiding this comment.
Note that the added test case in kml.cc demonstrates the existing code flying to the wrong place half way around the world. In working a another enhancement I got so sick of GE flying to the wrong side of the world I made a detour (this PR) to fix it.
The small changes in latitude are not due to finite precisions arithmetic, they are due to the span of longitude in the bounding box which moves the midpoint along a great circle diameter towards the pole.
Also note the code currently has COMPARE_BEARING_TO_GRTCIRC defined, which matches our grtcirc calculations, but uses a spherical earth with the equatorial radius. If we correct(undefine) that and use nvectors instead of grtcirc in the future a bunch of reference files will need to be tweaked.
| return longitude_degrees; | ||
| } | ||
|
|
||
| // great circle distance in radians |
There was a problem hiding this comment.
Sentence casing and punctuation, please.
There was a problem hiding this comment.
I pushed another commit with improvements to capitalization and spelling.
| return result; | ||
| } | ||
|
|
||
| // great circle distance in meters |
There was a problem hiding this comment.
Timid suggestion. ("No" is OK and I know we have a million other places already suffering from this...)
Instead of everything being a double should we have classes for Radians, Degrees and Feet or Meters along with overloads that convert between them? That would let us strongly typecheck arguments so we don't swap a Lat with a Lon or a degrees with a radian again. A 5_meters could convert to feet, but not to degrees.
Maybe c++ proposal P1935 is relevant or at least inspirational, but that's been punted to C++23 at least. We probably need a much simpler subset.
I'll also accept "sounds good, but I don't care enough to change it". :-) Things like "class Waypoint should store an NVector instead of a lat/lon tuple" fall into that same category.
There was a problem hiding this comment.
I note that with
NVector n1;
NVector n2;
n1+n2 has type Vector3D, not NVector.
The non-explicit single parameter constructors for NVector and PVector are how we can get an NVector back. Of course clang-tidy will point out they are non-explicit single parameter constructors. I fooled around with this, even making Vector3D a template class, but didn't get anywhere I liked.
|
There is another old related issue. The bounding box longitudes approach -180 and 180 for a track like sim.csv that crosses the anitmeridian, where they should approach the max of the negative longitudes and the min of the positive longitudes. This effects our range calculation. |
|
What I believe will be a superior solution is draft PR #771 |
The kml bounding box had problems around the antimeridian, that were unsuccessfully mitigated in 2011. Instead of fixing the obvious mistake in that mitigation we use nvectors to compute the actual center of the bounding box.
A test case is added that shows the gross error around the antimeridian.
Nvectors will be useful to avoid problems at discontinuities and singularities that are inherent in the latitude, longitude coordinate system. Our implementation has gone extensive testing that isn't apparent in this commit. Planned enhancements will use more of the capability of nvectors.
We also restore some lost whitespace in gc_logs data. The whitespace change in LineStyles.kml is a vertical tab that is handled differently than the distant past.