set the correct program link parameters

[bosvensson1]

Currently, Open MW's shader manager has to be aware of shader implementation details to set the associated linking parameters and sets unneeded linking parameters when unaware of the requirements. With these changes we pass an osg::Program template from the outside that specifies the required linking parameters.

[AnyOldName3]

To me this looks like it increases coupling, although I agree the way things worked before was gross. Are there other approaches we could take here? IIRC, layout qualifiers might not be an option because of our GLSL version, but if they are, they'd at least let us restrict the coupling to the things already doomed to be coupled, and the shader manager wouldn't need to care about any of this.

[glassmancody]

The layout identifier isn't standard until GLSL 140, location in 330, and binding in 420 so will still be stuck with setting these OSG side.

[bosvensson1]

In my opinion this PR reduces coupling. The shader manager no longer depends on a constant from another component.

[AnyOldName3]

In my opinion this PR reduces coupling. The shader manager no longer depends on a constant from another component.

In that respect, it does, but now lots of other things directly interact with the shader manager and we've had to introduce the concept of program templates, which things are coupled with.

Does OSG perhaps have something for sorting out binding by name? I know OpenGL lets you query the binding points for things by name, so I'd have thought Robert would have made OSG track that (whether it used the driver's defaults or came up with its own) and then implemented a mechanism to wire things up.

This feels like a dumb idea, but we could also maybe alternatively do something with our shader preprocessor to make it strip out unsupported layout(location = ...) specifications (so the driver never sees them), parses them, and then calls the necessary methods on the program to get the same result as we would have if they'd been supported.

[bosvensson1]

we've had to introduce the concept of program templates, which things are coupled with.

Program templates are weakly coupled at most. We default to nullptr templates.

This feels like a dumb idea, but we could also maybe alternatively do something with our shader preprocessor to make it strip out unsupported layout(location = ...) specifications (so the driver never sees them), parses them, and then calls the necessary methods on the program to get the same result as we would have if they'd been supported.

With this quiet complex approach we end up with nonstandard shader files incompatible with other engines.

[bosvensson1]

Does OSG perhaps have something for sorting out binding by name? I know OpenGL lets you query the binding points for things by name, so I'd have thought Robert would have made OSG track that (whether it used the driver's defaults or came up with its own) and then implemented a mechanism to wire things up.

Osg's Vertex Attribute Aliasing can bind vertex attributes automatically that are referred by their fixed function name in the shader. We need to enable this functionality globally. If we do so, we may have unneeded tracking overhead when groundcover uses shaders while objects do not. Ubo can not be bound with this approach - there is no equivalent fixed function feature.

[AnyOldName3]

Does OSG perhaps have something for sorting out binding by name? I know OpenGL lets you query the binding points for things by name, so I'd have thought Robert would have made OSG track that (whether it used the driver's defaults or came up with its own) and then implemented a mechanism to wire things up.

Osg's Vertex Attribute Aliasing can bind vertex attributes automatically that are referred by their fixed function name in the shader. We need to enable this functionality globally. If we do so, we may have unneeded tracking overhead when groundcover uses shaders while objects do not. Ubo can not be bound with this approach - there is no equivalent fixed function feature.

Well that's a bit pants. It basically forces us to do something with lots of coupling, use a newer GLSL version with location, or make fake location support.

[bosvensson1]

Is there any reason we did not merge this PR?

[psi29a]

I don't think anyone tested it.

[AnyOldName3]

It wasn't merged because it looked to me like it massively increased coupling and I wanted a more experienced engineer to take a look.

[glassmancody]

I've noticed we get a bunch of uniform block LightBufferBinding has no binding spam when loading cells with this change.

[bosvensson1]

This message is thrown by OSG here, but it does not include any context unfortunately. It would be worth to investigate the particular programs that cause the message. Perhaps we have enabled some lighting calculations in programs that do not in fact use them.

[glassmancody]

Perhaps we have enabled some lighting calculations in programs that do not in fact use them.

I'm unclear what you mean by lighting calculations that we don't use.

What's happening now is that osg::Progam::addBindUniformBlock is not called for all necessary programs. All object, terrain, groundcover, and water shader programs will need this binding, and at first glance I don't think that's handled here. I can look into this more thoroughly later, need to review the changes this PR actually made for first time.

Edit: after looking over this implementation I see how this all works now with default program, the issue is something else entirely (see my comment below).

[glassmancody]

Oh I see, OSG program copy does not copy uniform binding locations...

https://github.com/openscenegraph/OpenSceneGraph/blob/master/src/osg/Program.cpp#L102

Bug, maybe. You'll need to manually copy over the bindings, unless I've missed something obvious why these shouldn't be copied (though we definitely need them copied for our purpose).

Copy in context here:

https://github.com/OpenMW/openmw/pull/3110/files#diff-5440a2d4e8e4411ab7dcfe91e4b88b639bab9fbd506d25cadbcc28fb82ca1ac5R185

And here

https://github.com/OpenMW/openmw/pull/3110/files#diff-b39bedad868bead978caf42442d199eeac01dcf196ead354f2ac366382539197R348

[bosvensson1]

Thank for you this great research. I will need to refactor this PR a little to work around this apparent bug outside of our control.