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.

Sign up for GitHub

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

Jump to bottom

Require geospatial component #539

Merged
merged 6 commits into from
Mar 15, 2022
Merged

Require geospatial component #539

merged 6 commits into from
Mar 15, 2022

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Jan 19, 2022 •
edited
Loading

🦟 Bug fix

Summary

Adds the new geospatial component (gazebosim/gz-common#267) as it is needed by heightmaps.

Depends on gazebo-release/gz-common5-release#4

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@iche033 iche033 added the 🌱 garden Ignition Garden label Jan 19, 2022
@iche033
add gdal dep
67c7079 
Signed-off-by: Ian Chen <[email protected]>
@iche033
revert dep
effaefd 
Signed-off-by: Ian Chen <[email protected]>
chapulina
chapulina approved these changes Jan 19, 2022
View reviewed changes
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.

Some includes may also need to change if gazebosim/gz-common#289 goes in

@scpeters
Update header locations
ce615f8 
Signed-off-by: Steve Peters <[email protected]>
@scpeters
Copy link
Member

Some includes may also need to change if ignitionrobotics/ign-common#289 goes in

ce615f8

@scpeters
Copy link
Member

the windows build is currently failing since the gdal component is not installed in CI, so the Heightmap headers aren't available ( gazebosim/gz-common#267 (comment) )

@scpeters scpeters mentioned this pull request Jan 19, 2022
Closed
@scpeters
Copy link
Member

the windows build is currently failing since the gdal component is not installed in CI, so the Heightmap headers aren't available ( ignitionrobotics/ign-common#267 (comment) )

testing a release-tools update in #540

@scpeters
Copy link
Member

@osrf-jenkins run tests please
I reconfigured the windows job to use the ci_matching_branch/require_geospatial branch of release-tools to see if that fixes the windows build

@scpeters
Copy link
Member

we can consider reverting the ign-common PRs since the downstream packages weren't ready. That was my mistake as I suggested that we were ready to merge gazebosim/gz-common#267

@scpeters scpeters mentioned this pull request Jan 19, 2022
Merged
nkoenig
nkoenig reviewed Jan 19, 2022
View reviewed changes
CMakeLists.txt
@@ -36,7 +36,7 @@ set(IGN_MATH_VER ${ignition-math7_VERSION_MAJOR})
#--------------------------------------
# Find ignition-common
ign_find_package(ignition-common5 REQUIRED
COMPONENTS graphics events)
Copy link
Contributor

Choose a reason for hiding this comment

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

The geospatial component in ign-common is optional, while geospatial is required here. Is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm if we make the geospatial component optional here, we will need to make heightmaps in ign-rendering optional as well and only build it if the geospatial component is available. We'll likely need to do the same in ign-gazebo.

Copy link
Member

Choose a reason for hiding this comment

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

should we revert the ign-common PR for now while we sort this out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok that sounds good as this will require a bigger change

Copy link
Member

Choose a reason for hiding this comment

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

gazebosim/gz-common#291

Copy link
Contributor

Choose a reason for hiding this comment

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

In a previous f2f meeting with @chapulina , we decided against making GDAL an optional component because it was going to add a layer of difficulty managing

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I remember also discussing this at the weekly meeting. GDAL will be required for heightmaps. Ignition Physics already has a separate heightmap component, so only that component needs to depend on geopspatial. But Ignition Rendering doesn't have that separation and with the current architecture I think it may be difficult to break that apart.

@scpeters scpeters mentioned this pull request Jan 19, 2022
Merged
7 tasks
@jennuine
Copy link
Contributor

Requires: gazebosim/gz-common#292

@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Mar 1, 2022
@chapulina chapulina mentioned this pull request Mar 3, 2022
Closed
@chapulina chapulina mentioned this pull request Mar 15, 2022
Merged
7 tasks
jennuine and others added 2 commits March 14, 2022 18:07
@jennuine @chapulina
Update heightmap size & added DEM example (#560)
ffacdc5 
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
@chapulina
merged from main
0975461 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Mar 15, 2022
@chapulina
Copy link
Contributor

We can ignore the failing Bionic CI, it's being removed in

@chapulina
Copy link
Contributor

CI is happy!

@chapulina chapulina merged commit 8cc48ac into main Mar 15, 2022
@chapulina chapulina deleted the require_geospatial branch March 15, 2022 17:05
@chapulina chapulina mentioned this pull request Mar 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennuine jennuine jennuine left review comments

@scpeters scpeters scpeters left review comments

@nkoenig nkoenig nkoenig left review comments

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
🌱 garden Ignition Garden
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@iche033 @scpeters @jennuine @chapulina @nkoenig