[3.x] Fix Rare GL Bug caused by Particle Systems #61632
Conversation
|
Is this fix also needed for GLES2? Could we alternatively unbind all texture slots when starting to render a viewport? I was just wondering if this same bug could occur in other circumstances than particle systems, and we hadn't spotted it yet, and maybe we could fix all such bugs in the same way. One effect of this PR could be if you were rendering e.g. 100 of the same particle system, it would bind / unbind the same texture 100 times. It is a bit of a tricky one though. Another alternative is some kind of wrapper for keeping track of which slots are in use and their current values, and unbinding the unused slots, and preventing redundant changes. 🤔 What are your thoughts @clayjohn ? |
This bug almost definetly exists in other systems. @clayjohn has raised this to me. I think cleaning the textures up at the start or end of a viewport render is okay but I think it would be better to identify what render commands are leaving stuff bound and unbinding them intelligently during the render loop.
The actual execution of the unbind and whether there is a performance increase to 'not unbinding after each particle use' is probably driver dependent and a performance increase is not clearly gained there. In general, I think a robust, simple renderer system will bind it's resources, render its thing(s) and then unbind those resources. I think it's a mistake in this renderer to leave this bound. A much more powerful renderer would batch it's draw calls more intelligently, but just leaving random stuff bound is not the way to do it. Godot 3.x should heavily prioritize correctness rather than optimizations as 4.0 is meant to be the future proof place for large work like this. |
Possibly, I'm not sure what the cost would be if we were to be unbinding 1000 already unbound texture slots. My guess is most if not all drivers would be able to no-op and perform no work, but it could lead to pathological edge cases on devices with low-quality drivers. My original suggestion was to track max texture id usage over the course of a single renderloop (that is per viewport) and then unbind all the used slots. So the unbind would be called once per frame per viewport. Essentially making each viewport clean up after itself. I think that we can intelligently identify places that require a full unbind and then ensure that all active texture slots are unbound. For example, the issue here is from the engine leaving a screen_texture bound to texture slot 2 and then rendering to the screen_texture later in the frame (or perhaps early in the next frame). To work around this problem while minimizing binds/unbinds, we could track max texture slot usage, and then call a full unbind right before switching to a new framebuffer. That would ensure that everytime we render to a framebuffer there is no chance that that framebuffer's texture is currently bound. At the same time, it would result in fewer unbinds then unbinding after every draw command. |
This was assuming the maximum would be 8 or 16 slots (we could look on the GLES database to find the actual numbers involved, I think only 8 are mandated in GLES2). Keeping track of the maximum we use is also a good option if the number of slots is higher than this, or using a wrapper to keep track of texture slots for this (I've used this approach in the past in other projects).
Also agree that unbinding on situations like rendering a new viewport seems more sensible than the current approach in this PR, as it should catch all such bugs instead of just this particular one. |
This is a known, precise fix for a known, precise individual bug which should afford some value. I think 'catch all' systems as described should be considered a last resort. Locating the unbound textures and unbinding them per command feels better to me, rather than using the blunt instrument of 'unbind everything'. Maybe I'm wrong to feel that way. Can we identify a second instance of this class of bug? Is there only one other example? Should we similarly precisely fix that single other bug?
I dont think it matters but just to clarify, I'm fairly certain that the bug that this PR fixes was not related to |
| @@ -6557,6 +6557,9 @@ void RasterizerStorageGLES3::update_particles() { | |||
| } | |||
|
|
|||
| Material *material = material_owner.getornull(particles->process_material); | |||
| int texture_count = 0; | |||
| RID *textures = nullptr; | |||
There was a problem hiding this comment.
This doesn't seem needed in this outer scope in the end?
The idea isn't to unbind everything, its to unbind what was used since the last framebuffer switch. In general, I agree that it is best to fix the specific bug. But in this case, this isn't a bug that arises between commands. Unbinding all resources after a command is guaranteed to unbind more often than necessary (although depending on drivers, that may not actually be a big deal). Again, the bug comes from having a framebuffer's texture left bound while rendering to that framebuffer. If we unbind up to the max texture slot used by a given Viewport, then we guarantee that the framebuffer texture is never left bound when it comes time to render to that framebuffer.
You are correct, I mispoke, when I wrote screen_texture I meant the backbuffer texture associated with a framebuffer (which is the current viewports SCREEN_TEXTURE). When accessed from another Viewport, it isn't correct to call it the screen_texture anymore. |
|
@jasonwinterpixel This is what I had in mind, clayjohn@baf691a Is there any way if you could test out that commit and let me know if it resolves your bug as well? I wasn't able to reproduce locally. |
On a philosophy point - although it may seem strange, the policy as far as I see in Godot is that just because something fixes a bug, doesn't guarantee it will be merged. The emphasis is very much on finding "the right fix" (by consensus with maintainers etc). It does seem to help keep the codebase as a whole more concise and sensible, and less what might be considered "a patchwork of bugfixes" which can happen to some projects, making them more difficult to maintain. The flipside of course is that it can mean bugs remain open a little longer. This happens to all of us BTW regularly. What often happens making a PR is we don't see the forest for the trees (and often reduz comes up with a better way of fixing as he has the best overview). Anyway that isn't to say the approach in this PR has no value, it shows very well the cause and solution to the problem. And we don't yet know if the other approaches will fix it! 😀 |
akien-mga
added
the
cherrypick:3.5
|
@clayjohn that branch of yours looked fine to me, and as we haven't heard back, maybe you could make a PR, as it should (in theory anyway 😁 ) fix the issue without the risk of unnecessary API calls. |
akien-mga
removed
the
cherrypick:3.5
|
Superseded by #80481. Thanks for the contribution! |
This PR unbinds particle systems textures after use.
I spoke with @clayjohn on the rocket chat about this.
If you have a particle system that uses more than 1 texture, it will bind two textures.
It will never unbind that second texture.
If you render a viewport later on in your scene, on webgl, you will receive a GL error:
On non webgl platforms, the system merely liberally allows the texture to be bound, and returns all black if it is sampled.
This happens because the particle system left a texture bound. This PR fixes it.
There is some possibility this bug is present on master, but I don't know how the renderer works on master so I cant comment.