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

Supply spherical coords when loading DEMs #1556

Merged
merged 15 commits into from
Jul 29, 2022

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Jun 24, 2022

🎉 New feature

Summary

This PR makes Physics.cc supply the spherical coordinates object to the DEM object, so that the width and heights of the dem model are computed accurately.

Depends on these PRs, and CI won't pass until then :

Example

An example world has been added with MOON_SCS tag

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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.

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

Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
@chapulina chapulina added the 🌱 garden Ignition Garden label Jun 25, 2022
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Jun 30, 2022
@adityapande-1995 adityapande-1995 marked this pull request as ready for review July 25, 2022 16:36
@chapulina chapulina added the enhancement New feature or request label Jul 25, 2022
Signed-off-by: Aditya <aditya050995@gmail.com>
@adityapande-1995 adityapande-1995 force-pushed the aditya/supply_spherical_coords_to_dem branch from 898fa81 to d406c73 Compare July 27, 2022 20:23
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #1556 (8edc674) into main (f2f8eca) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

❗ Current head 8edc674 differs from pull request most recent head c5ea0cb. Consider uploading reports for the commit c5ea0cb to get more accurate results

@@            Coverage Diff             @@
##             main    #1556      +/-   ##
==========================================
- Coverage   63.61%   63.59%   -0.02%     
==========================================
  Files         330      330              
  Lines       25931    25943      +12     
==========================================
+ Hits        16495    16498       +3     
- Misses       9436     9445       +9     
Impacted Files Coverage Δ
include/gz/sim/rendering/SceneManager.hh 100.00% <ø> (ø)
src/rendering/SceneManager.cc 27.67% <0.00%> (-0.10%) ⬇️
src/systems/physics/Physics.cc 64.98% <0.00%> (-0.19%) ⬇️
src/rendering/RenderUtil.cc 39.05% <75.00%> (+0.09%) ⬆️

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 f2f8eca...c5ea0cb. Read the comment docs.

@jennuine jennuine removed the needs upstream release Blocked by a release of an upstream library label Jul 29, 2022
@adityapande-1995
Copy link
Contributor Author

I don't see a test case for SceneManager explicitly, so not sure how codecov can be improved here.

Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine
Copy link
Contributor

Instead of installing the model through gz-sim, I think it's better to upload the model to fuel and use the <include> link which I've done here: fe40f95

Here is the uploaded moon model: https://app.gazebosim.org/OpenRobotics/fuel/models/Moon%20DEM

Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

It would be nice to get a second pair of 👀 since I've made updates but if we can't by EOD, I'd say lets go ahead and merge this

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.

Just some minor nitpicks, LGTM once they're addressed

include/gz/sim/rendering/SceneManager.hh Outdated Show resolved Hide resolved
src/rendering/SceneManager.cc Outdated Show resolved Hide resolved
tutorials/spherical_coordinates.md Outdated Show resolved Hide resolved
Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Aditya <aditya050995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants