Port custom materials to Hlms pieces

[darksylinc]

Desired behavior

Right now specific passes such as the Segmentation Camera iterates through all objects, changes their material to a custom low level materials, and renders that.

This works but has a few issues:

• It cannot handle all details that HlmsPbs/HlmsUnlit does (e.g. HW skeletal animation)
• It doesn't handle custom Hlms implementations (such as Terra's)
• It can be slow to change the materials so often (switch it on every object to render, then restore them back)
• It isn't API portable (it needs one material per API i.e. D3D11/Metal/GL)

Ogre +2.1 supports Hlms pieces, which allows users to add custom code that can be run before/during/after our default code runs.

This solves all outstanding issues:

• It's fast
• No need to switch materials (we'd still need to iterate through objects to disable those unwanted, unless we use visibility flags instead, but that's out of scope of this ticket)
• It's API portable
• It works with (nearly?) all Hlms implementations
• All features supported by the underlying implementation that affect vertex position (i.e. HW skeletal animation) are automatically supported.

Alternatives considered

This is the recommended way.

Implementation suggestion

I'll be the one implementing this feature

Additional context

[iche033]

yeah that'll be nice go with the recommended way. Originally, we tried material schemes but couldn't get it to work in ogre 2.1. We then resorted to this workaround and have been doing it in many places of our code, e.g. MaterialSwitcher used by selection buffer. So it would nice to go with the proper solution.

[darksylinc]

The changes are currently on https://github.com/darksylinc/ign-rendering/tree/matias-hlmsCustom

A few takes:

• It's going to be slighter harder than I realized; because we have to send custom data per object (not a huge deal given that it's only 1 set of float4)
• Iteration through objects is still needed so we can set those values to the object. What changes is that we no longer need to swap out materials (the rest of the benefit remain, e.g. API cross portability, vertex shader-based deformation such as HW skinning, etc)
  • Not iterating through objects every time we need the segmentation camera looks doable but that'd be a refactor from Ign side, rather than interface between Ign and Ogre. I won't be addressing that now; and anyone could do it

[darksylinc]

Segmentation camera is already working! I couldn't believe it worked on first try. I was suspicious at first and... it wasn't working :rofl: But after a few fixes it's working!

Things missing:

• [x] Unlit
• [x] Terrain
• [x] Harmonize with other Hlms customizations (Ogre2IgnHlmsCustomizations)
• Reuse same code for other types of cameras?
  • [x] Ogre2MaterialSwitcher is used in Ogre2SelectionBuffer
  • [x] Ogre2LaserRetroMaterialSwitcher
  • [x] Ogre2ThermalCameraMaterialSwitcher
• [x] Add removeCustomParameter
• [x] Fix Ogre2GpuRays forcing IORM_SOLID_COLOR when rendering to the particle texture (likely also a performance optimization)
  • The problem is that cameraPreRenderScene and Post get executed for both passes, not just the first one

Update:

While going through this PR I noticed that currently HlmsPbsTerraShadows (used for terrain shadows) and Ogre2IgnHlmsCustomizations (used for Ogre2GpuRays*) are clashing:

hlmsPbs->setListener(this->dataPtr->hlmsPbsTerraShadows.get());
hlmsPbs->setListener(&this->dataPtr->hlmsCustomizations); // err.. only one can be set

as they were merged as separate Pull Requests.

I will harmonize them in the PR.

But as for refactoring Ogre2SelectionBuffer, Ogre2ThermalCameraMaterialSwitcher, Ogre2LaserRetroMaterialSwitcher and possibly others; I will very likely split them into another PR.

Otherwise a single PR could become too large (touches too many files, a silly bug could be introduced that would block the entire merge) making it hard to review. Porting these classes to use Hlms should be straightforward once the first PR is accepted.

(*) As for Fortress, personally I'd say HlmsPbsTerraShadows has higher priority than Ogre2IgnHlmsCustomizations, because the latter was fixing a bug that was also fixed in several ways IIRC. I'm not sure how necessary it is now. Worst case scenario, Ogre2GpuRays could call hlmsPbs->setListener when it needs to render, then when it's done it restores HlmsPbsTerraShadows as the listener

[darksylinc]

I have a question (I think I'm already answering myself):

I wish to rename the class Ogre2IgnHlmsCustomizations to Ogre2IgnHlmsSphericalClipMinDistance because that's the only thing it does and will do (there is already another class at a higher level that groups all modules together)

The rename should happen in the PR? If I do it now, it would make review harder, not to mention a whole file (Ogre2IgnHlmsCustomizations.hh and Ogre2IgnHlmsCustomizations.cc) would have to be renamed.

Since it's a simple rename, it should be split into its own standalone PR, right?

Update: I've decided to put it in its own standalone PR

[darksylinc]

Ogre2IgnHlmsCustomizations rename is in branch matias-hlmsCustom2

[iche033]

(*) As for Fortress, personally I'd say HlmsPbsTerraShadows has higher priority than Ogre2IgnHlmsCustomizations, because the latter was fixing a bug that was also fixed in several ways IIRC. I'm not sure how necessary it is now. Worst case scenario, Ogre2GpuRays could call hlmsPbs->setListener when it needs to render, then when it's done it restores HlmsPbsTerraShadows as the listener

I just tried disabling the Ogre2IgnHlmsCustomaizations in Ogre2GpuRays.cc by doing the test mentioned in https://github.com/ignitionrobotics/ign-rendering/pull/356#issuecomment-923490966, and found that the lidar returns are now different so we will need to do the workaround of calling setListener when render is needed.

Since it's a simple rename, it should be split into its own standalone PR, right?

Update: I've decided to put it in its own standalone PR

yep that sounds good to me.

[darksylinc]

I just tried disabling the Ogre2IgnHlmsCustomaizations in Ogre2GpuRays.cc by doing the test mentioned in #356 (comment), and found that the lidar returns are now different so we will need to do the workaround of calling setListener when render is needed.

Understood (note that it affects Fortress).

I do have something to raise though: Enabling Ogre2IgnHlmsCustomizations makes GL_DEPTH_CLAMP pointless:

void Ogre2DepthCamera::Render()
{
#ifndef _WIN32
  if (useGL)
    glEnable(GL_DEPTH_CLAMP);
#endif

 ...
#ifndef _WIN32
  if (useGL)
    glDisable(GL_DEPTH_CLAMP);
#endif
}

Thus if you remove it, you will probably get the same results as having Ogre2IgnHlmsCustomizations on 99% of the time.

[iche033]

for Ogre2DepthCamera I remember it was important to have the GL_DEPTH_CLAMP, otherwise the integration test fails. We currently don't have hlms customizations for this particular class, and I think you don't need to touch that class for the work you're using right?

[darksylinc]

This ticket is almost done except we're missing porting Ogre2ThermalCameraMaterialSwitcher and Terrain. Terrain should be a piece of cake.

Ogre2ThermalCameraMaterialSwitcher will be a more difficult because it has two materials: solid colour (easy) and a texture variation. I'm tempted to leave the texture variation using a V1 material. That will depend on how easy/difficult it ends up being.

[darksylinc]

OK Ogre2ThermalCameraMaterialSwitcher is done. It had 3 paths: solid colour, textured heat source, and background (can be solid or textured).

Solid colour and background are covered via Hlms pieces. However textured heat source I preferred to keep it as a Material because it makes more sense: Using Hlms would be more work, there could be little to gain, and there's potential for introducing more bugs than I'd be fixing.

The things that remain are:

• Terrain (easy)
• Adding removeCustomParameter
  • This last one item I just added it. The thing is I realized that after we're done we must call removeCustomParameter. By doing that, if we combine multiple cameras (e.g. Thermal + Segmented) we may miss bugs because one camera set a custom parameter while the other did nothing. This mistake is present in all previous branches (i.e. Fortress, Edifice, etc) and I just want to harden our codebase to detect incorrect usage.

By the next weekend the final PR should be ready for submission. Let's hope #534 is merged by then :)

[azeey]

This is fixed by #545.