Geospatial component for heightmaps (2nd try) #292
Conversation
Signed-off-by: Jenn Nguyen <[email protected]> Co-authored-by: Steve Peters <[email protected]> Co-authored-by: Ian Chen <[email protected]>
This simplifies the logic required for packaging. Update Migration guide Signed-off-by: Steve Peters <[email protected]>
scpeters
changed the title
Codecov Report
@@ Coverage Diff @@
## main #292 +/- ##
==========================================
+ Coverage 76.75% 77.12% +0.37%
==========================================
Files 75 76 +1
Lines 10328 10529 +201
==========================================
+ Hits 7927 8121 +194
- Misses 2401 2408 +7
Continue to review full report at Codecov.
|
|
there is a test failure with from windows: from macOS knowing very little about DEM files, I tried running the "Corner Coordinates" section seems promising, but computing differences doesn't give values that match up with the test expectations of approximately |
|
Per VC, there may not be a reliable way to calculate world width/height because of projections and coordinate transformations. Instead will try to set world dimensions with projected measurements since we can get meters per pixel. This is possible with already projected DEMs (projected coordinate system), need to be sure it's possible when they are in a geographic coordinate system. |
There's no reason for this to be non-const, and it causes some issues in ign-physics, so make it non-const while it is possible to change the API. Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
…tionrobotics/ign-common into ci_matching_branch/retry_geospatial Signed-off-by: Jenn Nguyen <[email protected]>
|
A workaround to load non-Earth DEMs fa605bd and I've created an issue regarding computing the world dimensions #311. To visually test, you can run the example from gazebosim/gz-rendering#560 I believe this PR is ready for review |
looks like there's a just a few windows compiler warnings to fix |
|
Hmmm, do you know what changed windows ci? These warnings didn't appear when we merged #267 |
I think windows CI wasn't fully working when we merged #267. I believe it landed in the following build: |
Signed-off-by: Jenn Nguyen <[email protected]>
|
@osrf-jenkins run tests please |
|
it looks like the windows build fails on one jenkins node and passes on the other:
cc @j-rivero |
There was a problem hiding this comment.
The examples work for me 👍 I noticed that a lot of errors and warnings are printed for the Moon's DEM, even though it seems to have been loaded correctly:
ERROR 1: PROJ: proj_create_operations: Source and target ellipsoid do not belong to the same celestial body
ERROR 6: Cannot find coordinate operations from `PROJCRS["Moon2000_npole",BASEGEOGCRS["GCS_Moon",DATUM["Moon_2000",ELLIPSOID["Moon_2000_IAU_IAG",1737400,0,LENGTHUNIT["metre",1,ID["EPSG",9001]]]],PRIMEM["Reference_Meridian",0,ANGLEUNIT["degree",0.0174532925199433,ID["EPSG",9122]]]],CONVERSION["unnamed",METHOD["Polar Stereographic (variant B)",ID["EPSG",9829]],PARAMETER["Latitude of standard parallel",90,ANGLEUNIT["degree",0.0174532925199433],ID["EPSG",8832]],PARAMETER["Longitude of origin",0,ANGLEUNIT["degree",0.0174532925199433],ID["EPSG",8833]],PARAMETER["False easting",0,LENGTHUNIT["metre",1],ID["EPSG",8806]],PARAMETER["False northing",0,LENGTHUNIT["metre",1],ID["EPSG",8807]]],CS[Cartesian,2],AXIS["easting",south,ORDER[1],LENGTHUNIT["metre",1]],AXIS["northing",south,ORDER[2],LENGTHUNIT["metre",1]]]' to `EPSG:4326'
[Err] [Dem.cc:280] Unable to transform terrain coordinate system to WGS84 for coordinates (0,0)
[Wrn] [Dem.cc:154] Failed to automatically compute DEM size. Assuming non-Earth DEM.
If these can be ignored, I suggest making them all warnings instead of having an error there.
My suggestions below are specific to the DEM I'm trying to load (still trying, haven't succeeded yet), but I imagine other users may run into the same issues.
| double max = -ignition::math::MAX_D; | ||
| for (auto d : this->dataPtr->demData) | ||
| { | ||
| if (d > noDataValue) |
There was a problem hiding this comment.
While testing this PR with a DEM that uses NaN as the no data value, I noticed that this logic doesn't handle NaNs well, because any comparison to NaN will always return false.
How about something like this:
for (auto d : this->dataPtr->demData)
{
// All comparisons to NaN return false, so guard against NaN NoData
if (!std::isnan(noDataValue) && d <= noDataValue)
continue;
if (!std::isfinite(d))
continue;
if (d < min)
min = d;
if (d > max)
max = d;
}| { | ||
| OGRSpatialReference sourceCs; | ||
| OGRSpatialReference targetCs; | ||
| OGRCoordinateTransformation *cT; | ||
| double xGeoDeg, yGeoDeg; | ||
|
|
||
| // Transform the terrain's coordinate system to WGS84 | ||
| char *importString = strdup(this->dataPtr->dataSet->ProjectionRef()); | ||
| const char *importString | ||
| = strdup(this->dataPtr->dataSet->GetProjectionRef()); |
There was a problem hiding this comment.
I'm testing this with a DEM that doesn't have a projection ref. I think we could catch this here instead of passing an empty string to importFromWkt. Something like this:
const char *importString
= strdup(this->dataPtr->dataSet->GetProjectionRef());
if (importString == nullptr || importString[0] == '\0')
{
ignwarn << "Projection coordinate system undefined." << std::endl;
return false;
}
I've sent gazebo-tooling/release-tools#657. Bad news is that our vcpkg snapshot is too old to work (mirrors already removed the required versions from our snapshot). I don't think that the problem is a blocker to merge this PR. |
🎉 New feature
Second attempt at adding
geospatialcomponent.Summary
Our first attempt at the
geospatialcomponent did not havegdalinstalled in macOS or Windows CI, and we didn't notice because thegeospatialcomponent is an optional dependency. This had broken ign-rendering builds, so #267 and #289 were reverted in #291. These dependency issues have been resolved in osrf/homebrew-simulation#1790, gazebo-tooling/release-tools#626, and gazebo-tooling/release-tools#624, so let's try re-adding these pull requests.This needs to be merged in sync with downstream pull requests to ign-rendering, ign-physics, and ign-gazebo.
Test it
UNIT_Dem_TESTis the primary test added in this pull request, but this should also be tested with downstream branches.Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.