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

Heightmap (rendering only) #487

Merged
merged 14 commits into from
Feb 9, 2021
Merged

Heightmap (rendering only) #487

merged 14 commits into from
Feb 9, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Dec 15, 2020

Requires:

This works for me when compiling Ogre 1 from source (tag 1.9.1), but not when using debs. With debs, the heightmap only shows if the heightmap is created during RenderUtil::Init as well as its current place. I'm still investigating. Fixed on ign-rendering.

This also needs to be updated as I change the ign-rendering implementation so heightmap inherits from geometry. Done

This is working now:

image

This is ready for review.

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina added rendering Involves Ignition Rendering needs upstream release Blocked by a release of an upstream library 🏢 edifice Ignition Edifice labels Dec 15, 2020
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina marked this pull request as ready for review December 18, 2020 00:40
@chapulina chapulina requested a review from iche033 as a code owner December 18, 2020 00:40
@nkoenig nkoenig changed the base branch from main to ign-gazebo5 December 23, 2020 23:34
@chapulina chapulina changed the base branch from ign-gazebo5 to main January 5, 2021 00:28
@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #487 (e83f38d) into main (2f49e6f) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
+ Coverage   77.34%   77.47%   +0.12%     
==========================================
  Files         213      213              
  Lines       11967    12009      +42     
==========================================
+ Hits         9256     9304      +48     
+ Misses       2711     2705       -6     
Impacted Files Coverage Δ
src/Conversions.cc 83.13% <100.00%> (+0.97%) ⬆️
src/SimulationRunner.cc 93.85% <0.00%> (+1.08%) ⬆️

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 2f49e6f...e83f38d. Read the comment docs.

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.

code changes look good. Just waiting for green CI

@chapulina chapulina mentioned this pull request Feb 4, 2021
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Feb 4, 2021
@iche033
Copy link
Contributor

iche033 commented Feb 4, 2021

there are some compile errors in windows CI for other unrelated classes. Looks like it's happening in other PRs too.

@chapulina
Copy link
Contributor Author

there are some compile errors in windows CI

That's been fixed 🎉 (ignore the test failures 🙃 )

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.

🎉

@chapulina chapulina merged commit 215fe82 into main Feb 9, 2021
@chapulina chapulina deleted the chapulina/5/heightmap branch February 9, 2021 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice rendering Involves Ignition Rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants