Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geospatial component for heightmap & DEMs #267

Merged
merged 16 commits into from
Jan 19, 2022
Merged

Conversation

jennuine
Copy link
Contributor

@jennuine jennuine commented Nov 19, 2021

🎉 New feature

Summary

Test it

/path/to/build/bin/UNIT_Dem_TEST

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@jennuine jennuine self-assigned this Nov 19, 2021
@jennuine jennuine added the 🌱 garden Ignition Garden label Nov 19, 2021
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! It would be good to test compiling a downstream library that uses heightmap against this branch to make sure all the includes are found and everything links correctly.

There are some CI failures, probably due to a different GDAL version on Bionic?

math::SphericalCoordinates::Distance(upLeftLat, upLeftLong,
lowLeftLat, lowLeftLong);
try
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was ported straight from Gazebo classic, but can this try block be narrower, including just the lines that throw an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone ahead and removed the try/catch block and replaced it with an error instead. Also, instead of continuing through the code, it returns when the error is reached 6f6769a

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we had to throw an exception in gazebo11 since the GeoReference method returned void and its API was already frozen. I notice that you changed that API to return bool in this PR, which is much better. 👍

gzthrow("Illegal coordinates. You are asking for the elevation in (" <<
_x << "," << _y << ") but the terrain is [" << this->Width() <<
" x " << this->Height() << "]\n");
throw std::invalid_argument(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider getting rid of these exceptions and printing errors instead. We don't throw exceptions often.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated it to an error instead and am returning inf if reached. What do you think? 6f6769a

@jennuine
Copy link
Contributor Author

Thanks for the review!

It would be good to test compiling a downstream library that uses heightmap against this branch to make sure all the includes are found and everything links correctly

I was able to run ign gazebo heightmap.sdf in a Garden workspace without any issues. Does this suffice?

There are some CI failures, probably due to a different GDAL version on Bionic?

Yes, this is the likely cause but are we dropping Bionic support for Garden?

@chapulina
Copy link
Contributor

I was able to run ign gazebo heightmap.sdf in a Garden workspace without any issues. Does this suffice?

Cool! So you updated all locally libraries to use ign-common5?

Yes, this is the likely cause but are we dropping Bionic support for Garden?

Ahh good call. I didn't realize that would bear fruit so early. We need to update CI

@jennuine
Copy link
Contributor Author

Cool! So you updated all locally libraries to use ign-common5?

Yup, specifically ign-fuel-tools, ign-gazebo, ign-gui, ign-launch, ign-physics, ign-rendering, and ign-sensors.

Ahh good call. I didn't realize that would bear fruit so early. We need to update CI

Should I create an issue somewhere?

@chapulina chapulina mentioned this pull request Nov 19, 2021
8 tasks
@chapulina
Copy link
Contributor

Should I create an issue somewhere?

I created some issues:

Also see #270

@chapulina chapulina self-requested a review November 29, 2021 19:57
@scpeters
Copy link
Member

scpeters commented Dec 8, 2021

I think it definitely makes sense to move Dem.hh to the geospatial component, but I'm not sure about moving HeightmapData.hh and ImageHeightmap.hh out of graphics. gdal isn't used by ImageHeightmap, but moving this code to geospatial would add that dependency to this code.

@jennuine
Copy link
Contributor Author

jennuine commented Dec 9, 2021

I think it definitely makes sense to move Dem.hh to the geospatial component, but I'm not sure about moving HeightmapData.hh and ImageHeightmap.hh out of graphics. gdal isn't used by ImageHeightmap, but moving this code to geospatial would add that dependency to this code.

That makes sense. @chapulina do you have any thoughts before I make the changes?

@chapulina
Copy link
Contributor

I agree that requiring GDAL to use image heightmaps may be undesired, but when we have a geospatial component, I think it will be confusing for users that not all heightmap classes are in it, and some are actually in graphics.

The reason to add the geospatial component was to avoid adding that dependency to all graphics users, but we chose that name because we wanted the component to be semantically meaningful, and not just referent to a dependency. We need to draw the line at some point and inevitably some users will always get a bit more than they actually need. That's true for other ign-common components and other Ignition libraries too. Also note that all our "internal" users (that is, all Ignition libraries that will use this: rendering and physics) will support DEMs anyway.

I'm leaning towards being more semantically precise instead of trying to reduce the dependency surface for hypothetical users.

@jennuine
Copy link
Contributor Author

jennuine commented Dec 9, 2021

I'm leaning towards being more semantically precise instead of trying to reduce the dependency surface for hypothetical users.

I agree, I think this will avoid confusion for users.

@scpeters
Copy link
Member

scpeters commented Dec 9, 2021

I agree that requiring GDAL to use image heightmaps may be undesired, but when we have a geospatial component, I think it will be confusing for users that not all heightmap classes are in it, and some are actually in graphics.

The reason to add the geospatial component was to avoid adding that dependency to all graphics users, but we chose that name because we wanted the component to be semantically meaningful, and not just referent to a dependency. We need to draw the line at some point and inevitably some users will always get a bit more than they actually need. That's true for other ign-common components and other Ignition libraries too. Also note that all our "internal" users (that is, all Ignition libraries that will use this: rendering and physics) will support DEMs anyway.

I'm leaning towards being more semantically precise instead of trying to reduce the dependency surface for hypothetical users.

if users depend on the geospatial component, then they will get ImageHeightmap either way. I just wanted to point out that removing the currently functional heightmap classes from graphics will require migration action for downstream packages. If we proceed in this manner, then we need to update the migration guide to reflect this

@chapulina
Copy link
Contributor

I just wanted to point out that removing the currently functional heightmap classes from graphics will require migration action for downstream packages.

Ok, that's a good point, users currently depending on graphics would need to update to depend on geospatial.

At least this PR is targeting main. Ideally we'd tick-tock the change, but in this case I think it could get complicated.

@scpeters
Copy link
Member

I just wanted to point out that removing the currently functional heightmap classes from graphics will require migration action for downstream packages.

Ok, that's a good point, users currently depending on graphics would need to update to depend on geospatial.

At least this PR is targeting main. Ideally we'd tick-tock the change, but in this case I think it could get complicated.

so we should update the migration guide?

@chapulina
Copy link
Contributor

so we should update the migration guide?

That seems the most natural thing to me. When users update their find_package calls from comon4 to common5, they also need to add the geospatial component. I know it's not ideal, but some things are tough to tick-tock.

Signed-off-by: Jenn Nguyen <[email protected]>
@jennuine
Copy link
Contributor Author

I have updated Migration.md 90e55a1

iche033 and others added 5 commits January 7, 2022 18:12
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
geospatial/src/Dem.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment about error messages

Signed-off-by: Jenn Nguyen <[email protected]>
@scpeters
Copy link
Member

I think this will require an update to https://github.com/ignition-release/ign-common5-release when it is merged

@jennuine
Copy link
Contributor Author

Please wait to merge, I may add a small update.

@jennuine
Copy link
Contributor Author

Decided not to add the update. Going to merge. Thanks for the reviews!

@jennuine jennuine merged commit 28eb6c7 into main Jan 19, 2022
@jennuine jennuine deleted the jennuine/geospatial branch January 19, 2022 00:33
scpeters added a commit to gazebo-release/gz-common5-release that referenced this pull request Jan 19, 2022
Follow-up to gazebosim/gz-common#267

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit to scpeters/ign-common that referenced this pull request Jan 19, 2022
@scpeters scpeters mentioned this pull request Jan 19, 2022
7 tasks
mjcarroll pushed a commit that referenced this pull request Jan 19, 2022
* Revert "Move geospatial headers to subfolder (#289)"

This reverts commit e0a5490.

Signed-off-by: Steve Peters <[email protected]>

* Revert "Geospatial component for heightmap & DEMs (#267)"

This reverts commit 28eb6c7.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit that referenced this pull request Jan 21, 2022
Signed-off-by: Jenn Nguyen <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
chapulina pushed a commit to gazebo-release/gz-common5-release that referenced this pull request Mar 15, 2022
* geospatial component

Follow-up to gazebosim/gz-common#267

Signed-off-by: Steve Peters <[email protected]>

* Remove geospatial.hh from -core-dev package

Signed-off-by: Steve Peters <[email protected]>

* fix dependencies in control file

Signed-off-by: Steve Peters <[email protected]>

* common5-dev package depend on geospatial-dev

Signed-off-by: Steve Peters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants