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

Add Heightmap class #388

Merged
merged 6 commits into from
Jan 5, 2021
Merged

Add Heightmap class #388

merged 6 commits into from
Jan 5, 2021

Conversation

chapulina
Copy link
Contributor

Resolves #382.

Added a C++ API for heightmap geometries that matches the XML spec.

I don't plan on changing the PR, but I'd like to see it working with ign-gazebo before merging, so I'll keep it as a draft while we work on the other parts.

Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina requested a review from scpeters October 13, 2020 04:17
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Oct 13, 2020
@chapulina chapulina linked an issue Oct 13, 2020 that may be closed by this pull request
@chapulina chapulina self-assigned this Oct 13, 2020
Signed-off-by: Louise Poubel <[email protected]>
@scpeters
Copy link
Member

scpeters commented Nov 6, 2020

looks ok to me; I'll look into how to add it to ign-physics; it will probably be similar to how meshes are handled, using ign-common

@chapulina
Copy link
Contributor Author

I'll look into how to add it to ign-physics; it will probably be similar to how meshes are handled, using ign-common

In case it helps, this is how I'm using common::HeightmapData in ign-rendering.

@chapulina
Copy link
Contributor Author

I'm working on the ign-gazebo PR and I have the need for some API to add textures and blends to a heightmap object. We don't do that much in the SDF DOM API, the only place where I saw Add functions is in sdf::Actor. Given that people have been discussing the API design (#292), are there new guidelines for this use case?

If it's still undecided, I guess I could leave this as it is and use the Element API to construct an object instead...

@chapulina chapulina marked this pull request as ready for review December 17, 2020 02:53
@chapulina
Copy link
Contributor Author

chapulina commented Dec 17, 2020

This PR is working with gazebosim/gz-sim#487. I still have some details to fix on that one, but I think the API on this one is settled, so I'm opening it for review, @scpeters.

On a related note, because ign-rendering doesn't directly depend on SDF, and I needed a class there to hold heightmap data, I ended up making an almost exact copy of sdf::Heightmap for ignition::rendering::HeightmapDescriptor on gazebosim/gz-rendering#180. It's worth mentioning that similar data is also present in ignition::msgs::HeightmapGeom. I ticketeted an issue about all this duplication with some ideas: gazebosim/gz-sim#494

@codecov-io
Copy link

codecov-io commented Dec 17, 2020

Codecov Report

Merging #388 (1b737a5) into sdf9 (0bca996) will increase coverage by 0.24%.
The diff coverage is 97.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##             sdf9     #388      +/-   ##
==========================================
+ Coverage   86.43%   86.68%   +0.24%     
==========================================
  Files          60       61       +1     
  Lines        9304     9516     +212     
==========================================
+ Hits         8042     8249     +207     
- Misses       1262     1267       +5     
Impacted Files Coverage Δ
src/Geometry.cc 89.38% <61.53%> (-3.62%) ⬇️
src/Heightmap.cc 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 0bca996...1b737a5. Read the comment docs.

@chapulina chapulina merged commit 32d96a6 into sdf9 Jan 5, 2021
@chapulina chapulina deleted the chapulina/heightmap branch January 5, 2021 00:28
doisyg pushed a commit to doisyg/sdformat that referenced this pull request Jan 6, 2021
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Guillaume Doisy <[email protected]>
This was referenced Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geometry C++ object lacks API's for Heightmaps
3 participants