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

Implement Hlms custom pieces swap for Objects & Terrain #545

Merged
merged 17 commits into from
Feb 1, 2022

Conversation

darksylinc
Copy link
Contributor

@darksylinc darksylinc commented Jan 23, 2022

🦟 Bug fix

Fixes #504
Fixes #444

Summary

The 3rd and final PR to ticket #504 !

After this PR:

  • Terra now works with all Cameras (Thermal, SelectionBuffer, Segmentation, Laser) as documented in ticket Heightmaps don't work properly with segmentation camera #444 (backporting this to Fortress w/out breaking ABI could be a ton of work unfortunately)
  • Fixes various misc bugs of wrong values used in these cameras by regular objects when their properties weren't set (documented in Potential bug: Wrong value stuck in Ogre2GpuRays #521, technically it fixes that one, but the fix can be backported so the ticket can will open until these versions are fixed or EoL)
  • Deformation works correctly under these cameras (e.g. skeletal animation and morph target)
    • Only exception is Thermal camera when using heatSignature texture. This was not ported to Hlms pieces and we remain swapping a regular low level material
  • Swaps the low level material for all cameras, not just a few of them (as documented in Incorrect sensor data: Low level materials and other cameras #544; however it doesn't fix the underlying issue that we don't respect the original material's vertex shader deformation). This can be fixed in older versions as well (w/out breaking ABI).
  • Increases test coverage by throwing an error (and crashing) if an object tries to render without a color set while in IORM_SOLID_COLOR and IORM_SOLID_THERMAL_COLOR_TEXTURED modes. Previously Gazebo would render colour to non-colour render targets and the unit tests wouldn't always catch it.

Notes:

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.

Also fix issue where objects would be rendered with garbage retro value

Signed-off-by: Matias N. Goldberg <[email protected]>
Signed-off-by: Matias N. Goldberg <[email protected]>
Use fast path for background objects in Ogre2ThermalCamera

Signed-off-by: Matias N. Goldberg <[email protected]>
Now there are no need for "fast" and "slow" paths in Ogre2ThermalCamera

Signed-off-by: Matias N. Goldberg <[email protected]>
This makes the engine more robust by being able to catch potential bugs.

Also Ogre2ThermalCamera forgot to set mode back to IORM_SOLID_COLOR

Signed-off-by: Matias N. Goldberg <[email protected]>
Signed-off-by: Matias N. Goldberg <[email protected]>
Also fix IORM_SOLID_COLOR not working for Unlit

Signed-off-by: Matias N. Goldberg <[email protected]>
Signed-off-by: Matias N. Goldberg <[email protected]>
Ogre2LaserRetroMaterialSwitcher was activating
IORM_SOLID_COLOR during the depth pass AND a particle pass.

The listener should not do that during the particle pass.
Switched to a CompositorWorkspaceListener instead of Camera::Listener to
identify the correct pass where it should be enabled.

Signed-off-by: Matias N. Goldberg <[email protected]>
The feature was recently added to the main branch.
Now tests are passing again

Signed-off-by: Matias N. Goldberg <[email protected]>
Signed-off-by: Matias N. Goldberg <[email protected]>
@darksylinc darksylinc requested a review from iche033 as a code owner January 23, 2022 00:18
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jan 23, 2022
@darksylinc darksylinc changed the title Matias hlms custom3 Implement Hlms custom pieces swap Jan 23, 2022
@darksylinc darksylinc changed the title Implement Hlms custom pieces swap Implement Hlms custom pieces swap for Objects & Terrain Jan 23, 2022
@codecov
Copy link

codecov bot commented Jan 23, 2022

Codecov Report

Merging #545 (409b9a2) into main (30f9a5e) will decrease coverage by 0.10%.
The diff coverage is 54.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #545      +/-   ##
==========================================
- Coverage   53.37%   53.26%   -0.11%     
==========================================
  Files         211      213       +2     
  Lines       20830    21152     +322     
==========================================
+ Hits        11117    11267     +150     
- Misses       9713     9885     +172     
Impacted Files Coverage Δ
...c/terrain/Terra/include/Terra/Hlms/OgreHlmsTerra.h 100.00% <ø> (ø)
ogre2/src/terrain/Terra/include/Terra/Terra.h 0.00% <ø> (ø)
ogre2/src/terrain/Terra/src/Terra.cpp 0.00% <0.00%> (ø)
ogre2/src/Ogre2IgnHlmsTerraPrivate.cc 42.85% <42.85%> (ø)
ogre2/src/Ogre2ThermalCamera.cc 82.18% <48.07%> (-6.95%) ⬇️
ogre2/src/Ogre2IgnHlmsPbsPrivate.cc 77.88% <58.33%> (-4.47%) ⬇️
ogre2/src/Ogre2MaterialSwitcher.cc 76.28% <61.11%> (-8.13%) ⬇️
ogre2/src/Ogre2GpuRays.cc 91.59% <65.88%> (-3.90%) ⬇️
ogre2/src/Ogre2SegmentationMaterialSwitcher.cc 62.50% <67.08%> (+2.76%) ⬆️
ogre2/src/Ogre2IgnHlmsTerraPrivate.hh 100.00% <100.00%> (ø)
... and 5 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 30f9a5e...409b9a2. 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.

lots of stuff to go through :) I did a quick first pass and some testing.

I created sensor_heightmap.sdf world file for testing in ignition gazebo and confirms that heightmap now appears in segmentation camera, lidar, and thermal camera (using fixed temperature). Looks promising!

sensor_heightmap

@@ -1286,6 +1421,9 @@ void Ogre2GpuRays::PostRender()
Ogre::TextureBox box = image.getData(0u);
float *bufferTmp = static_cast<float *>(box.data);

// TODO(anyone): It seems wasteful to have gpuRaysBuffer at all
// We should be able to convert directly from bufferTmp to gpuRaysScan
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

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.

Did more tests and confirmed that particles are working fine (testing using sensors_particle_scarttering2.sdf), and lidar spherical near clipping is also working (tesed using robots_near_clip.sdf).

@iche033 iche033 merged commit d90a20c into gazebosim:main Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants