Fix various issues with Ogre2GpuRays #522
Conversation
There were multiple bugs that had to be fixed: - Objects which had no laser_retro set (or an invalid one) would be rendered using regular lighting PBS shaders; causing the retro value to contain garbage instead of the intended default value of 0 - Performance regression in 993bb0b: When ported to Ogre 2.2, ignition would render the scene 4 times instead of 2 (twice for objects, twice for particles). Depth buffer data was created through a 2nd redundant pass. This is inefficient as the depth buffer data is already available from the 1st pass - Bug regression in 993bb0b: In the pass for normal objects, ignition would first use IGN_VISIBILITY_ALL flags for colour, but it would use 0x01000000 for depth. This creates the wrong mix of data because an occluder that had visibility set to 0x01000000 would use the correct depth but the retro value of the object behind it. This bug is implicitly fixed by the previous item (colour and depth data are now obtained from the same pass instead of using multi pass) - PFG_R8_UNORM is nowhere enough to contain all possible laser retro values and would quantize too much. Increased precision to PFG_R16_UNORM - Affects gazebosim#521. The problem is fixed for Fortress by always overriding setCustomParameter, but likely the fix should be backported to previous versions. Signed-off-by: Matias N. Goldberg <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-rendering6 #522 +/- ##
==================================================
- Coverage 54.44% 54.39% -0.05%
==================================================
Files 198 198
Lines 20107 20084 -23
==================================================
- Hits 10947 10925 -22
+ Misses 9160 9159 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
ah now this refreshes my memory of the retro + visibility mask issue.
I think with these changes, we should be able to remove this todo comment about using an even more complicated workaround to fix retro values for objects that do not have one set.
https://github.com/ignitionrobotics/ign-rendering/blob/fa4b52cbe513282b7c2921e28d631d601a6fc318/ogre2/src/Ogre2GpuRays.cc#L910-L919
This is a much better fix.
I'll create a follow-up PR to remove this todo item and add a small check in the integration test for zero retro values.
|
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 |
There were multiple bugs that had to be fixed:
rendered using regular lighting PBS shaders; causing the retro value to
contain garbage instead of the intended default value of 0
When ported to Ogre 2.2, ignition would render the scene 4 times instead
of 2 (twice for objects, twice for particles). Depth buffer data was
created through a 2nd redundant pass. This is inefficient as the depth
buffer data is already available from the 1st pass
pass for normal objects, ignition would first use IGN_VISIBILITY_ALL
flags for colour, but it would use 0x01000000 for depth. This creates
the wrong mix of data because an occluder that had visibility set to
0x01000000 would use the correct depth but the retro value of the object
behind it. This bug is implicitly fixed by the previous item (colour and
depth data are now obtained from the same pass instead of using multi
pass)
values and would quantize too much. Increased precision to PFG_R16_UNORM
setCustomParameter, but likely the fix should be backported to previous
versions.
Signed-off-by: Matias N. Goldberg [email protected]
🦟 Bug fix
Affects #521
Fixes other bugs without ticket
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.