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 missing terrain shadows casted on objects #547

Merged
merged 2 commits into from
Jan 25, 2022

Conversation

darksylinc
Copy link
Contributor

@darksylinc darksylinc commented Jan 23, 2022

🦟 Bug fix

No ticket assigned; but it was mentioned in #536 (comment)

Summary

Issue was incorrect listener setup due to bad merge.

ign-rendering7 has a more robust system to handle all listeners. For
Fortress this fix will do.

Garden will already have a better fix after PR #545 is merged.

The code that should not be carried over to Garden was surrounded by #ifdef macros and an assert was placed. Hopefully that will help reducing merge conflict mistakes.

Signed-off-by: Matias N. Goldberg [email protected]

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@darksylinc darksylinc requested a review from iche033 as a code owner January 23, 2022 19:09
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jan 23, 2022
Issue was incorrect listener setup due to bad merge.

ign-rendering7 has a more robust system to handle all listeners. For
Fortress this fix will do.

Signed-off-by: Matias N. Goldberg <[email protected]>
@darksylinc darksylinc force-pushed the matias-missingShadows branch from 17f3325 to 9c4c31c Compare January 23, 2022 19:10
@codecov
Copy link

codecov bot commented Jan 23, 2022

Codecov Report

Merging #547 (f9b8d84) into ign-rendering6 (f305d78) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           ign-rendering6     #547      +/-   ##
==================================================
+ Coverage           54.37%   54.47%   +0.09%     
==================================================
  Files                 198      198              
  Lines               20096    20099       +3     
==================================================
+ Hits                10928    10948      +20     
+ Misses               9168     9151      -17     
Impacted Files Coverage Δ
ogre2/src/Ogre2RenderEngine.cc 78.58% <ø> (-0.05%) ⬇️
ogre2/src/Ogre2GpuRays.cc 95.51% <100.00%> (+0.03%) ⬆️
...a/src/Hlms/PbsListener/OgreHlmsPbsTerraShadows.cpp 58.00% <0.00%> (+34.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 f305d78...f9b8d84. 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.

looks good to me.

@iche033 iche033 merged commit fae20cd into gazebosim:ign-rendering6 Jan 25, 2022
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

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

Successfully merging this pull request may close these issues.

3 participants