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

Reduce default heightmap sampling from 2 to 1 #459

Merged
merged 7 commits into from
Jan 26, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Jan 12, 2021

There's no good reason to subsample by default, see discussion in gazebosim/gz-rendering#180 (comment).

This also needs to be changed in Heightmap.hh once that's merged forward, so I'll keep this PR as a draft until:

  • 9 ➡️ 10 #458 is merged
  • then a 10 ➡️ 11 PR is merged
  • this PR is updated to also change the default sampling on the C++ code

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added the 🏢 edifice Ignition Edifice label Jan 12, 2021
@chapulina chapulina marked this pull request as draft January 12, 2021 03:22
@codecov-io
Copy link

codecov-io commented Jan 12, 2021

Codecov Report

Merging #459 (ebd9ec2) into master (a721bba) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #459   +/-   ##
=======================================
  Coverage   88.10%   88.10%           
=======================================
  Files          66       66           
  Lines        9940     9940           
=======================================
  Hits         8758     8758           
  Misses       1182     1182           
Impacted Files Coverage Δ
src/Heightmap.cc 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 a721bba...ebd9ec2. Read the comment docs.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina marked this pull request as ready for review January 14, 2021 19:30
@chapulina
Copy link
Contributor Author

This PR is ready for review

@iche033
Copy link
Contributor

iche033 commented Jan 14, 2021

changes look good. Just need to update the heightmap test expectations to fix the failing tests

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

@scpeters , I'd like to make sure you agree with this change from the physics point of view

@chapulina chapulina merged commit 54a69c5 into master Jan 26, 2021
@chapulina chapulina deleted the chapulina/11/heightmap_sampling branch January 26, 2021 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants