Replacing GenericShader with Magnum's Shaders::Flat

[mosra]

Motivation and Context

Exciting screenshots first ✨ -- before:

and after:

Yes, this changes absolutely nothing in the visual output, only uses Magnum's own Shaders::Flat for rendering instead of GenericShader -- the goal of this PR is to preserve current visual output while porting to Magnum's builtin functionality, in order to ensure no regression happened before we jump on more complex shaders.

Depends on #150 (and thus the reusing-depth-buffer branch), I'm waiting for at least one more review there before it lands, as it changes quite an important base functionality.

There's one feature of the GenericShader that still has to wait for changes on Magnum side and that's the primitive ID texturing used by PLY files. The relevant shader code is separated into a new PrimitiveIDTextured{Shader,Drawable} where it will temporarily live until Magnum can handle this as well.

How Has This Been Tested

Locally verified on GLB, PLY and House 3D models, tests are passing on my machine as well as the CI and there's no performance regression as far as I can tell, actually (slightly) the opposite:

[bigbike]

@mosra:
As you suggested in your comments at #132, do you think we should cache the uniform location of the "transformationProjectionMatrix" in the constructor? This function will be called in every frame by the draw().
The same for the "texSize".

[mosra]

Here I did just the most brainless copy of the original shader, trying to introduce as little changes (and potential bugs) as possible :) I'll be opening a cleanup PR for this shader (fixing also the webgl TODO) right once this is merged.

[bigbike]

I mentioned it in the other comment though.
I created a TextureBindingPointIndex in an independent header in #132 and plan to move such "TextureLayer" to it, so all the texture binding points are mutually exclusive. Do you think it is necessary?

[mosra]

(I'll answer all other comments properly later.) Making it global inevitably leads to using up all available binding points (some systems have as little as 8) with no more left to be used by new shaders. Having global dedicated binding points that are not reused by anything else is good for often-used data like lookup tables or glyph cache for text rendering. For diffuse texture not so much, I'd say, as it's unlikely to be reused by different shaders / draw calls in the same frame.

[msavva]

Agree with @mosra -- I don't see much value in defining global dedicated binding points in this case, and so I'd avoid the complexity of creating TextureBindingPointIndex.

[bigbike]

(I'll answer all other comments properly later.) Making it global inevitably leads to using up all available binding points (some systems have as little as 8) with no more left to be used by new shaders. Having global dedicated binding points that are not reused by anything else is good for often-used data like lookup tables or glyph cache for text rendering. For diffuse texture not so much, I'd say, as it's unlikely to be reused by different shaders / draw calls in the same frame.

@mosra: Yes, the binding points are limited, I do aware. I made this change because we actually do not have that many shaders (up to 8) in our system, and I agree with your comment in my ongoing PR, here I quote,

"It's generally advised to choose mutually exclusive texture units for each shader (unless it's expected to use the same textures in each), as you save on texture rebinding when switching between different shaders. So e.g. if the generic shader makes use of units 0-3, the PTex shader would take units 4 and 5."

We may have a case, the physical scene, where the scene is a replica model and physics-enabled objects are in glb format, that means, two different shaders will be used at the same time in the system.
Thanks!

[mosra]

The main goal is avoiding state changes as much as possible, because with GL (as opposed to Vulkan, e.g.), there's a lot of validation and generally unclear driver-specific work behind every call. With Vulkan the draw calls and state changes are put into a command buffer and reused across multiple frames, so there it doesn't matter as much.

If there's a possibility that multiple objects will use the same shader (and the same textures), it's best to draw them in succession -- so instead of let's say, drawing walls, then first physics box, then chairs, second physic box, then table, third box ... drawing the whole environment first (which is likely using the same shader and the same diffuse texture if coming from a single GLB), and then all physics-powered objects, because those are likely to be instanced, reusing the same mesh / texture / shader. Magnum's state tracker will then just reuse existing shader/texture/VAO binding instead of calling into GL for a texture / shader / VAO switch.

This can be achieved for example by grouping to multiple drawable groups (e.g. one drawable group for a shader). Or, another thing I did as an "invisible" background for this PR is design work to enable instanced drawing with builtin shaders, which would be especially useful for reducing state changes and draw call count for physics-enabled objects -- and that would solve this particular case as well.

[bigbike]

Grouping the similar drawables (e.g., the instanced cheese boxes) and thus reduce the state switching is a very good idea. The logic is quite similar to the case when rendering a single object. We often group and render the primitives by materials. Thank you for the suggestion!

Introducing the TextureBindingPointIndices is to reduce the conflict, and thus reduce the state changes. But I checked the code after my previous comment, and found we actually bind textures in every frame, in every drawables. We can improve it later by leveraging your aforementioned "grouping drawables".

For the common object format, such as .glb, .obj we can rely on the builtin shaders in magnum, but for specific models (not industry standard right now), such as Replica models, we will have to cover it by our simulator.

[mosra]

and found we actually bind textures in every frame, in every drawables

Magnum has a state tracker, so if it sees you are re-binding the same texture again, it won't redundantly call into GL. In other words, it's enough to just group by similar texture/shader but no need to add additional logic to avoid the bind() calls themselves -- Magnum already does that for you.

That said, I don't think it's needed to over-optimize this right now. We're not drawing thousands of objects every frame to be bound by API overhead. Just potential optimization opportunities to keep in mind for future features like phyiscs or arbitrary objects being added to the scene.

[msavva]

LGTM! 👍 Left a minor comment about a typo, otherwise good to merge imo.