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

Fix setting cast_shadows for visuals without material #1015

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Sep 8, 2021

Signed-off-by: Ian Chen ichen@osrfoundation.org

🦟 Bug fix

Summary

The <cast_shadows> SDF element has no effect for svisuals with no <material> element. The reason is that the cast shadows property change is not propagated to the parent submesh. This PR works around the issue by setting the material back to the submesh.

To test:

Download the Electrical Box. Modify the model.sdf file in your fuel cache, i.e.: ~/.ignition/fuel/fuel.ignitionrobotics.org/openrobotics/models/electrical box/3/model.sdf, and add the following to the <visual> SDF element.

 <cast_shadows>0</cast_shadows>

Launch ign-gazebo with this world and insert the Electrical Box model to see that it no longer casts shadows.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033 iche033 requested a review from jennuine September 8, 2021 05:45
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Sep 8, 2021
@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #1015 (6733cd6) into ign-gazebo3 (7b326dc) will increase coverage by 0.05%.
The diff coverage is n/a.

❗ Current head 6733cd6 differs from pull request most recent head e32b75c. Consider uploading reports for the commit e32b75c to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3    #1015      +/-   ##
===============================================
+ Coverage        77.80%   77.85%   +0.05%     
===============================================
  Files              221      221              
  Lines            12687    12687              
===============================================
+ Hits              9871     9878       +7     
+ Misses            2816     2809       -7     
Impacted Files Coverage Δ
...e/ignition/gazebo/detail/EntityComponentManager.hh 95.20% <0.00%> (-0.69%) ⬇️
src/Server.cc 84.93% <0.00%> (+0.60%) ⬆️
src/gui/plugins/plot_3d/Plot3D.cc 47.82% <0.00%> (+4.34%) ⬆️

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 7b326dc...e32b75c. Read the comment docs.

@chapulina chapulina added the rendering Involves Ignition Rendering label Sep 8, 2021
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.

Adding <cast_shadows>0</cast_shadows> to the visual works for me, but adding

<experimental:params>
  <visual element_id="link::visual">
    <cast_shadows action="add">0</cast_shadows>
  </visual>
</experimental:params>

to import_mesh.sdf still doesn't work (issue #1010).

@chapulina chapulina merged commit 5858ae6 into ign-gazebo3 Sep 8, 2021
@chapulina chapulina deleted the cast_shadows_no_material branch September 8, 2021 15:49
@jennuine
Copy link
Contributor

jennuine commented Sep 8, 2021

Adding <cast_shadows>0</cast_shadows> to the visual works for me, but adding

<experimental:params>
  <visual element_id="link::visual">
    <cast_shadows action="add">0</cast_shadows>
  </visual>
</experimental:params>

to import_mesh.sdf still doesn't work (issue #1010).

This is because //experimental:params is implemented in Dome

@chapulina
Copy link
Contributor

This is because //experimental:params is implemented in Dome

Ahhhh makes sense 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel rendering Involves Ignition Rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants