Heightmaps 🏔️ #218

chapulina · 2021-03-03T02:58:02Z

🎉 New feature

Closes #156

Summary

Add an attach heightmap feature and its implementation for DART.

The current implementation has a nice feature: there are hidden holes where objects can go through 🕳️ 🙂 It may be related to gazebosim/gazebo-classic#2838

Test it

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial: on ign-gazebo
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

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Make dependency for dartsim plugin. Signed-off-by: Steve Peters <[email protected]>

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

still needs more changes in CustomHeightmapShape Signed-off-by: Steve Peters <[email protected]>

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

Signed-off-by: Louise Poubel <[email protected]>

chapulina · 2021-03-17T20:24:12Z

The current implementation has a nice feature: there are hidden holes where objects can go through hole slightly_smiling_face

This is a problem in the ode collision checker, which is the default one. Using the bullet checker, the heightmap behaves as expected:

I tested the above with these branches:

main...chapulina/4/heightmap_detector, which includes this and Set collision detector and solver for DART #225
gazebosim/gz-sim@main...chapulina/5/heightmap_detector, which includes Heightmap physics (with DART) 🏔️ gz-sim#661 and Set collision detector and solver from SDF gz-sim#684

This PR is ready for review. CC @scpeters

Signed-off-by: Louise Poubel <[email protected]>

codecov · 2021-03-17T20:53:52Z

Codecov Report

Merging #218 (63cbe64) into ign-physics4 (a20dbba) will increase coverage by 0.14%.
The diff coverage is 80.70%.

@@               Coverage Diff                @@
##           ign-physics4     #218      +/-   ##
================================================
+ Coverage         75.26%   75.40%   +0.14%     
================================================
  Files               122      125       +3     
  Lines              5376     5433      +57     
================================================
+ Hits               4046     4097      +51     
- Misses             1330     1336       +6

Impacted Files	Coverage Δ
dartsim/src/SDFFeatures.cc	`66.39% <0.00%> (-0.83%)`	⬇️
dartsim/src/CustomHeightmapShape.cc	`82.60% <82.60%> (ø)`
dartsim/src/ShapeFeatures.cc	`40.36% <94.73%> (+7.70%)`	⬆️
...clude/ignition/physics/heightmap/HeightmapShape.hh	`100.00% <100.00%> (ø)`
...gnition/physics/heightmap/detail/HeightmapShape.hh	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a20dbba...63cbe64. Read the comment docs.

chapulina · 2021-03-24T20:13:24Z

Removed from beta, this can come in a minor release.

Signed-off-by: Louise Poubel <[email protected]>

chapulina · 2021-04-23T23:39:47Z

See gazebo-release/ign-physics4-release#9 for Debian packaging

chapulina · 2021-04-23T23:59:16Z

This is ready for review, @scpeters

scpeters

I'm going to make a change to ign-common to allow passing HeightmapData as a const reference to AttachHeightmapShape

scpeters · 2021-04-28T01:11:34Z

heightmap/include/ignition/physics/heightmap/HeightmapShape.hh

+      /// \param[in] _subSampling Increase sampling to improve resolution.
+      public: ShapePtrType AttachHeightmapShape(
+          const std::string &_name,
+          ignition::common::HeightmapData &_heightmapData,


I saw that you had to make this argument non-const in 923ae18 since HeightmapData::FillHeights is not a const function. I believe that could be a const function, so I'll go ahead and add a const version alongside the current one so we can make this argument const again

I'm going to make a change to ign-common to allow passing HeightmapData as a const reference to AttachHeightmapShape

HeightmapData is a pure virtual class, so I think it will be hard to add new API to it without breaking ABI. I have a band-aid workaround in 457c052, which shouldn't be too costly since the HeightmapData classes themselves are pretty thin. Let me know what you think.

There's a lot that bothers me about the HeightmapData class, I think it could be refactored in the future.

Signed-off-by: Louise Poubel <[email protected]>

heightmap/include/ignition/physics/heightmap/HeightmapShape.hh

dartsim/src/CustomHeightmapShape.cc

dartsim/src/CustomHeightmapShape.hh

dartsim/src/EntityManagement_TEST.cc

Signed-off-by: Louise Poubel <[email protected]>

ahcorde

  /usr/bin/find: '/github/workspace/tpe/include': No such file or directory
  /usr/bin/find: '/github/workspace/bullet/include': No such file or directory
  /github/workspace/dartsim/src/CustomHeightmapShape.cc:55:  Lines should be <= 80 characters long  [whitespace/line_length] [2]

scpeters

I finally finished looking over the changes you've made, and I just found one tiny thing that I will fix directly

dartsim/src/EntityManagement_TEST.cc

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

dartsim/src/CustomHeightmapShape.cc

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

scpeters · 2021-07-08T17:28:08Z

/usr/bin/find: '/github/workspace/tpe/include': No such file or directory
/usr/bin/find: '/github/workspace/bullet/include': No such file or directory
/github/workspace/dartsim/src/CustomHeightmapShape.cc:55: Lines should be <= 80 characters long [whitespace/line_length] [2]

I fixed the line length in 4a1edc3

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

scpeters · 2021-07-08T21:01:02Z

added some test expectations for CastToHeightmapShape in 63cbe64 to improve coverage

codecheck already fixed

Signed-off-by: Steve Peters <[email protected]> Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: Steve Peters <[email protected]>

scpeters and others added 8 commits February 25, 2021 18:11

Add heightmap component mirroring mesh component

37e1312

Make dependency for dartsim plugin. Signed-off-by: Steve Peters <[email protected]>

Copy CustomMeshShape.* to CustomHeightmapShape.*

77d7661

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

Start adding heightmap code

a4d07ab

still needs more changes in CustomHeightmapShape Signed-off-by: Steve Peters <[email protected]>

Latest prototyping, experimenting with API

e1172e2

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

fix compilation

2c8cf7d

Signed-off-by: Louise Poubel <[email protected]>

fix compilation again

acfbafc

Signed-off-by: Louise Poubel <[email protected]>

working, remove unnecessary bits

923ae18

Signed-off-by: Louise Poubel <[email protected]>

Merge branch 'main' into chapulina/4/heightmap

9789b8f

Signed-off-by: Louise Poubel <[email protected]>

chapulina added enhancement New feature or request DART DART engine labels Mar 3, 2021

chapulina requested a review from scpeters March 3, 2021 02:58

chapulina self-assigned this Mar 3, 2021

github-actions bot added the 🏢 edifice Ignition Edifice label Mar 3, 2021

tweaks

a19a66a

Signed-off-by: Louise Poubel <[email protected]>

chapulina mentioned this pull request Mar 3, 2021

Heightmap physics (with DART) 🏔️ gazebosim/gz-sim#661

Merged

8 tasks

chapulina mentioned this pull request Mar 16, 2021

Set collision detector and solver for DART #225

Merged

8 tasks

chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021

chapulina added 2 commits March 17, 2021 12:34

Support bullet detector

bf4ff30

Signed-off-by: Louise Poubel <[email protected]>

Add test, remove scale

e87ae73

Signed-off-by: Louise Poubel <[email protected]>

chapulina marked this pull request as ready for review March 17, 2021 20:24

chapulina requested a review from mxgrey as a code owner March 17, 2021 20:24

chapulina added 3 commits March 17, 2021 13:26

codecheck

a499b5f

Signed-off-by: Louise Poubel <[email protected]>

docs

dddb105

Signed-off-by: Louise Poubel <[email protected]>

fix

8b0257e

Signed-off-by: Louise Poubel <[email protected]>

Merge branch 'main' into chapulina/4/heightmap

9604f82

chapulina changed the base branch from main to ign-physics4 April 1, 2021 20:37

merged from ign-physics4

4827374

Signed-off-by: Louise Poubel <[email protected]>

chapulina mentioned this pull request Apr 23, 2021

Add heightmap component gazebo-release/ign-physics4-release#9

Merged

scpeters reviewed Apr 28, 2021

View reviewed changes

chapulina added 3 commits April 27, 2021 18:54

const HeightmapData

457c052

Signed-off-by: Louise Poubel <[email protected]>

Merge branch 'ign-physics4' into chapulina/4/heightmap

82f7e04

Signed-off-by: Louise Poubel <[email protected]>

Merged from ign-physics4

4afb649

Signed-off-by: Louise Poubel <[email protected]>

ahcorde requested changes Jun 30, 2021

View reviewed changes

chapulina added 2 commits June 30, 2021 19:31

Merge branch 'ign-physics4' into chapulina/4/heightmap

7fa2a3b

Signed-off-by: Louise Poubel <[email protected]>

PR feedback

736f04b

Signed-off-by: Louise Poubel <[email protected]>

chapulina requested a review from ahcorde July 1, 2021 02:38

ahcorde previously requested changes Jul 1, 2021

View reviewed changes

scpeters reviewed Jul 8, 2021

View reviewed changes

dartsim/src/EntityManagement_TEST.cc Outdated Show resolved Hide resolved

Expect 0 from ImageHeightmap::Load

e700b5b

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

scpeters reviewed Jul 8, 2021

View reviewed changes

dartsim/src/CustomHeightmapShape.cc Outdated Show resolved Hide resolved

shorten line length

4a1edc3

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

Test coverage of CastToHeightmapShape

63cbe64

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

scpeters approved these changes Jul 8, 2021

View reviewed changes

chapulina merged commit e21c8c9 into ign-physics4 Jul 8, 2021

chapulina deleted the chapulina/4/heightmap branch July 8, 2021 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heightmaps 🏔️ #218

Heightmaps 🏔️ #218

chapulina commented Mar 3, 2021 •

edited

Loading

chapulina commented Mar 17, 2021

codecov bot commented Mar 17, 2021 •

edited

Loading

chapulina commented Mar 24, 2021

chapulina commented Apr 23, 2021

chapulina commented Apr 23, 2021

scpeters left a comment

scpeters Apr 28, 2021

chapulina Apr 28, 2021

ahcorde left a comment

scpeters left a comment

scpeters commented Jul 8, 2021

scpeters commented Jul 8, 2021

Heightmaps 🏔️ #218

Heightmaps 🏔️ #218

Conversation

chapulina commented Mar 3, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

chapulina commented Mar 17, 2021

codecov bot commented Mar 17, 2021 • edited Loading

Codecov Report

chapulina commented Mar 24, 2021

chapulina commented Apr 23, 2021

chapulina commented Apr 23, 2021

scpeters left a comment

Choose a reason for hiding this comment

scpeters Apr 28, 2021

Choose a reason for hiding this comment

chapulina Apr 28, 2021

Choose a reason for hiding this comment

ahcorde left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

scpeters commented Jul 8, 2021

scpeters commented Jul 8, 2021

chapulina commented Mar 3, 2021 •

edited

Loading

codecov bot commented Mar 17, 2021 •

edited

Loading