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 skybox URI #1037

Merged
merged 12 commits into from
Jul 6, 2022
Merged

Add skybox URI #1037

merged 12 commits into from
Jul 6, 2022

Conversation

mabelzhang
Copy link
Collaborator

@mabelzhang mabelzhang commented Jun 3, 2022

🎉 New feature

Summary

Add <uri> tag to <sky> tag to allow specifying skybox texture via SDF.

Test it

CI passes on tests.

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: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jun 3, 2022
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang mabelzhang marked this pull request as ready for review June 3, 2022 08:06
@mabelzhang mabelzhang requested a review from nkoenig June 3, 2022 08:06
@nkoenig
Copy link
Contributor

nkoenig commented Jun 6, 2022

A few high-level notes:

  1. How about calling the new SDF tag cubemap_uri?
  2. You'll need to add the new tag to https://github.com/gazebosim/sdformat/blob/sdf12/sdf/1.9/scene.sdf#L13
  3. You'll also need to update the Sky::Load function to load the new tag.
  4. Finally, update the Sky::ToElement function to output the new element as well.

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang
Copy link
Collaborator Author

68f1041 does the above, I think. However, the tests still fail locally for me. I thought a previous fix made them pass locally, but maybe I compiled something else. I'm very puzzled why it doesn't like the ToElement test. I added lines in Load and ToElement. Am I doing it wrong?

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #1037 (caa2eef) into sdf12 (1ee33b8) will increase coverage by 20.88%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##            sdf12    #1037       +/-   ##
===========================================
+ Coverage   65.38%   86.27%   +20.88%     
===========================================
  Files           2      109      +107     
  Lines          26    16039    +16013     
===========================================
+ Hits           17    13837    +13820     
- Misses          9     2202     +2193     
Impacted Files Coverage Δ
src/Sky.cc 100.00% <100.00%> (ø)
src/EmbeddedSdf.cc
include/test_config.h
src/ScopedGraph.hh 98.86% <0.00%> (ø)
src/InterfaceModel.cc 100.00% <0.00%> (ø)
usd/src/usd_parser/USDJoints.cc 64.17% <0.00%> (ø)
usd/src/usd_parser/USDStage.cc 97.14% <0.00%> (ø)
src/Converter.cc 93.19% <0.00%> (ø)
usd/src/cmd/usd2sdf.cc 86.66% <0.00%> (ø)
src/Types.cc 100.00% <0.00%> (ø)
... and 102 more

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 1ee33b8...caa2eef. Read the comment docs.

@nkoenig
Copy link
Contributor

nkoenig commented Jun 9, 2022

I looks like the tests pass on CI. I'll try locally to double check.

sdf/1.9/scene.sdf Outdated Show resolved Hide resolved
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>

Co-authored-by: Nate Koenig <nkoenig@users.noreply.github.com>
@nkoenig nkoenig self-requested a review June 22, 2022 18:43
@mabelzhang
Copy link
Collaborator Author

Good to merge? Or do we need a review from the Steve/Addisu?

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.

For completeness, there are no accompanying gz-sim / gz-rendering PRs for this one, right?

src/Sky.cc Outdated Show resolved Hide resolved
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
@mabelzhang
Copy link
Collaborator Author

Right. I believe we only needed a new tag, and gzweb is doing the rest.

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang
Copy link
Collaborator Author

ABI check is failing on some random stuff... readXMLStream, SDF_SHARE_PATH, SDF_VERSION_PATH 🤔

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang
Copy link
Collaborator Author

CI passing after removing const 😅 Guess it has to be copy...

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@mabelzhang mabelzhang merged commit 819386b into sdf12 Jul 6, 2022
@mabelzhang mabelzhang deleted the mabelzhang/skybox_texture branch July 6, 2022 16:30
@chapulina
Copy link
Contributor

Ouch, it looks like this was merged with a commit instead of a squash-merge. That messes up the branch comparison and changelog generation. No need to go back and fix it, just something to keep in mind for next time.

@mabelzhang
Copy link
Collaborator Author

Ah shoot! Sorry. Haven't done core development for a long time...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants