Cleanup common Renderer methods #6186
Conversation
| if (Shader == null) | ||
| throw new InvalidOperationException("No shader bound."); |
There was a problem hiding this comment.
This guard is slightly out of left field... I'm hoping it's just there for general usage safety?
There was a problem hiding this comment.
Yes, it's for general usage safety. It's probably not strictly required.
| public void BindUniformBuffer(string blockName, IUniformBuffer buffer) | ||
| { | ||
| if (boundUniformBuffers.TryGetValue(blockName, out IUniformBuffer? current) && current == buffer) | ||
| return; | ||
|
|
||
| FlushCurrentBatch(FlushBatchSource.BindBuffer); | ||
| SetUniformBufferImplementation(blockName, buffer); | ||
|
|
||
| boundUniformBuffers[blockName] = buffer; | ||
| } |
There was a problem hiding this comment.
This change weirds me out. The buffer tracking is added to the base Renderer, but both renderer implementations still maintain their own boundUniformBuffers dictionaries? Is this intentional just for the type safety or something? It sure is going to cause memory overheads (although they're likely not relevant).
There was a problem hiding this comment.
It was just the simplest way for me to move the most common logic (dedupe/flush) to the base class.
Something to consider is that they can only be "bound" in DrawVertices. With the way things currently work, it's unsafe to do it earlier than that because there could be misordered events, for example:
shader.BindUniformBuffer()
shader.Bind()
// Or
shader.Bind()
shader.BindUniformBuffer()
At least, we don't explicitly state that the bind order must be shader -> uniform buffer...
The situation can probably be improved in the future, perhaps by storing the bound UBOs to the shader instead.
There was a problem hiding this comment.
Is the misordering issue a blocker for e.g. exposing Renderer's dictionary as protected and then using that in renderer implementations, rather than have the base renderer track as well as the implementations?
There was a problem hiding this comment.
That sounds backwards to me because I'm tending towards making more of this class private like I did with the global UBO. The remaining protected ones are even too much for me.
For implementers I think it makes more sense to be given abstract methods to store whatever state they need from, rather than to piece together the parts of Renderer that they need at some point...
| while (renderer.IsFrameBufferBound(this)) | ||
| Unbind(); | ||
|
|
||
| deleteResources(true); |
There was a problem hiding this comment.
This is just inlining the old method right? Commit message mentions "avoiding redirection" so I just want to make sure I'm understanding correctly and this isn't some lower-level concern.
There was a problem hiding this comment.
Yes, just inlining it.
There was a problem hiding this comment.
Just looking through things again, GLRenderer does the same thing. It might be worth moving this to the base Renderer class instead...
There was a problem hiding this comment.
Are you going to be trying for that? Not sure whether this PR is safe to re-review otherwise.
There was a problem hiding this comment.
Yep, I've moved it back & added a common method in Renderer. Actually, the implementation in master is a bit scary because it disposed the FBOs immediately rather than the 3-frame-delayed ScheduleDisposal...
Renderer.DrawVertices(), which is the method that binds the global UBO.globalUniformBufferprivate.Renderer.BindUniformBuffer(), which does the state tracking/de-duping of flushes around UBO bindings.