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

Update heightmap size & added DEM example #560

Merged
merged 4 commits into from
Mar 15, 2022
Merged

Conversation

jennuine
Copy link
Contributor

@jennuine jennuine commented Feb 11, 2022

🦟 Bug fix

Requires gazebosim/gz-common#292

Summary

Updated heightmapSizeZ calculation, which was not ported from gazebo-classic. Thanks to @iche033.

Also updated heightmap example to include DEMs:

cd examples
mkdir build && cd build
cmake ../
make
./heightmap dem

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.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

nice demo works for me.

One suggestion is to keep the arguments to the heightmap example consistent with other examples, i.e.

./heightmap [engine_name] [graphics_api]

One idea would be to look for an --dem arg, e.g.

# I think your code will pick ogre 1.x by default - maybe useful to add a msg about this
./heightmap --dem

# same behavior as above
./heightmap ogre --dem

# tries to use ogre2 with dem
./heightmap ogre2 --dem

# tries to use ogre2 + metal with dem
./heightmap ogre2 metal --dem

@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Mar 1, 2022
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.

The demo works for me too. I like @iche033 's suggestions for the example's arguments.

tutorials/21_heightmap.md Outdated Show resolved Hide resolved
@chapulina chapulina added the 🌱 garden Ignition Garden label Mar 14, 2022
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.

I've addressed mine and @iche033 's comments. I think this is good to go!

@chapulina
Copy link
Contributor

@osrf-jenkins run tests please!

@chapulina
Copy link
Contributor

Ahh, merging into #539 so we only iterate on CI in one place

@chapulina chapulina merged commit ffacdc5 into require_geospatial Mar 15, 2022
@chapulina chapulina deleted the jennuine/dem branch March 15, 2022 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants