WebGL bindBuffer & bufferSubData errors/corruption when using Tess::indices_mut() #483
Comments
This is a workaround for hadronized/luminance-rs#483
In the OpenGL 3.3 backend, there is an implementation that allows a buffer object to be created and bound to a given target (for instance `ARRAY_BUFFER`) and allows to rebind the buffer later to another target (for instance `ELEMENT_ARRAY_BUFFER`). That optimization is handy because it allows to dissociate data storage from data representation, allowing to send a bunch of bytes and then using the target as source of representation. Well, turns out that optimization doesn’t work in WebGL2, so this commit implements a way to keep track of what a buffer was created as and then re-use that information every time we need to bind the buffer. That should fix #483 (and probably other bugs no one has encountered yet). Some integration tests are needed to ensure the fix does what it should.
In the OpenGL 3.3 backend, there is an optimization that allows a buffer object to be created and bound to a given target (for instance `ARRAY_BUFFER`) and allows to rebind the buffer later to another target (for instance `ELEMENT_ARRAY_BUFFER`). That optimization is handy because it allows to dissociate data storage from data representation, allowing to send a bunch of bytes and then using the target as source of representation. Well, turns out that optimization doesn’t work in WebGL2, so this commit implements a way to keep track of what a buffer was created as and then re-use that information every time we need to bind the buffer. That should fix #483 (and probably other bugs no one has encountered yet). Some integration tests are needed to ensure the fix does what it should.
In the OpenGL 3.3 backend, there is an optimization that allows a buffer object to be created and bound to a given target (for instance `ARRAY_BUFFER`) and allows to rebind the buffer later to another target (for instance `ELEMENT_ARRAY_BUFFER`). That optimization is handy because it allows to dissociate data storage from data representation, allowing to send a bunch of bytes and then using the target as source of representation. Well, turns out that optimization doesn’t work in WebGL2, so this commit implements a way to keep track of what a buffer was created as and then re-use that information every time we need to bind the buffer. That should fix #483 (and probably other bugs no one has encountered yet). Some integration tests are needed to ensure the fix does what it should.
I prepared to test but ran into a compilation error that appears to be due to b607b1e not having updated the WebGL2 backend, and left a comment on #477 about it. |
In the OpenGL 3.3 backend, there is an optimization that allows a buffer object to be created and bound to a given target (for instance `ARRAY_BUFFER`) and allows to rebind the buffer later to another target (for instance `ELEMENT_ARRAY_BUFFER`). That optimization is handy because it allows to dissociate data storage from data representation, allowing to send a bunch of bytes and then using the target as source of representation. Well, turns out that optimization doesn’t work in WebGL2, so this commit implements a way to keep track of what a buffer was created as and then re-use that information every time we need to bind the buffer. That should fix #483 (and probably other bugs no one has encountered yet). Some integration tests are needed to ensure the fix does what it should.
|
That should be okay now @kpreid. :) |
|
Updated test report against 0bb49ae: there is no visual corruption, but the index buffer updates are not taking effect, and there are errors logged: apparently one for the vertex buffer mutation and one for the index buffer mutation (two different code paths that were both disabled to avoid this bug): |
|
I am going to rework the examples so that I can share them between all platforms. I have an application I’ve been playing with as an experiment already doing that so that should be rather easy. That should allow to showcase that kind of bug because right now I don’t have a reproducible way to do it but I think the interactive triangle example should be a good start. |
In the OpenGL 3.3 backend, there is an optimization that allows a buffer object to be created and bound to a given target (for instance `ARRAY_BUFFER`) and allows to rebind the buffer later to another target (for instance `ELEMENT_ARRAY_BUFFER`). That optimization is handy because it allows to dissociate data storage from data representation, allowing to send a bunch of bytes and then using the target as source of representation. Well, turns out that optimization doesn’t work in WebGL2, so this commit implements a way to keep track of what a buffer was created as and then re-use that information every time we need to bind the buffer. That should fix #483 (and probably other bugs no one has encountered yet). Some integration tests are needed to ensure the fix does what it should.
In the OpenGL 3.3 backend, there is an optimization that allows a buffer object to be created and bound to a given target (for instance `ARRAY_BUFFER`) and allows to rebind the buffer later to another target (for instance `ELEMENT_ARRAY_BUFFER`). That optimization is handy because it allows to dissociate data storage from data representation, allowing to send a bunch of bytes and then using the target as source of representation. Well, turns out that optimization doesn’t work in WebGL2, so this commit implements a way to keep track of what a buffer was created as and then re-use that information every time we need to bind the buffer. That should fix #483 (and probably other bugs no one has encountered yet). Some integration tests are needed to ensure the fix does what it should.
In the OpenGL 3.3 backend, there is an optimization that allows a buffer object to be created and bound to a given target (for instance `ARRAY_BUFFER`) and allows to rebind the buffer later to another target (for instance `ELEMENT_ARRAY_BUFFER`). That optimization is handy because it allows to dissociate data storage from data representation, allowing to send a bunch of bytes and then using the target as source of representation. Well, turns out that optimization doesn’t work in WebGL2, so this commit implements a way to keep track of what a buffer was created as and then re-use that information every time we need to bind the buffer. That should fix #483 (and probably other bugs no one has encountered yet). Some integration tests are needed to ensure the fix does what it should.
This relates to #483 for testing corruption of `indices_mut()`. The example showcases the bug when used on the `web` executor (no issue with the `desktop` one). This bug is fixed in the next commit.
The bug was due to still using `bind_array_buffer` when streaming buffer updates to the GPU. Relates to #483.
hadronized
added
the
awaiting-reporter
|
Still not fully working. Tested against e3c1628 : errors and visual corruption. There are fewer errors reported now; Chrome says only this (no stack trace): Firefox says, also with no stack trace: My relevant draw calls are like I've uploaded my patches to test against yours at https://github.com/kpreid/all-is-cubes/tree/lum-webgl-fix. |
|
I tried running your code but the application (WASM) doesn’t seem to initialize correctly. |
|
I've pushed a patch with It seems that different OpenGL implementations have different ideas of how much dead code analysis to do before declaring a uniform inactive, and macOS is less thorough here than (some) Linux. (I was, in one case, computing a value in the vertex shader to pass to the fragment shader, then ignoring it in the fragment shader.) |
|
I just noticed this is still tagged awaiting-reporter. Is there any more info you need? |
In the OpenGL 3.3 backend, there is an optimization that allows a buffer object to be created and bound to a given target (for instance `ARRAY_BUFFER`) and allows to rebind the buffer later to another target (for instance `ELEMENT_ARRAY_BUFFER`). That optimization is handy because it allows to dissociate data storage from data representation, allowing to send a bunch of bytes and then using the target as source of representation. Well, turns out that optimization doesn’t work in WebGL2, so this commit implements a way to keep track of what a buffer was created as and then re-use that information every time we need to bind the buffer. That should fix #483 (and probably other bugs no one has encountered yet). Some integration tests are needed to ensure the fix does what it should.
This relates to #483 for testing corruption of `indices_mut()`. The example showcases the bug when used on the `web` executor (no issue with the `desktop` one). This bug is fixed in the next commit.
The bug was due to still using `bind_array_buffer` when streaming buffer updates to the GPU. Relates to #483.
hadronized
removed
the
awaiting-reporter
|
I’m going to try to fix that but the repository you linked doesn’t build so I can’t go any further. I will merge the PR as it’s already fixing things and will try with your code later as I can’t make it work and can’t reproduce then. |
|
Also, I think I’ve found where the problem comes from. Creating a new buffer (here) in WebGL will always use the same |
|
I tested with commit ba6d169 and still got buffer errors. I'd like to work with you on figuring out why my code won't build for you — I know it'll build on macOS and Linux (and the GitHub CI servers) but maybe I forgot to document some prerequisite or something like that. Feel free to open an issue on https://github.com/kpreid/all-is-cubes/issues or contact me through Discord. Of course, another option is to reduce it to a smaller project, but it'd still have to have a wasm build. It should be possible to have a repro that fits into luminance-examples-web… |
|
There are less frequent errors but still errors/corruption (tested at commit 11b0e62 on master). Based on the comment in the PR ("Merging this as it’s already a fix, and I will continue fixing the buffer problems by redesigning them a bit.") I think this issue should still be open? |
|
It shouldn’t have been closed, sorry for that. |
Buffers are not really needed / used by people and are an implementation detail, as they are required for (currently) three things: - Vertex buffers (for `Tess`). - Index buffers (still for `Tess`). - And uniform buffers, which are known to be problematic because of the unsafe nature of using them and will require a redesign. So better get rid of the concept of buffers and expose directly the upper abstractions, such as slicing `Tess`, which is already supported and then won’t be a breaking change. The only case that breaks is when people want to build a `Tess` from GPU buffers, which is not going to be supported anymore. Relates to #483.
Buffers are not really needed / used by people and are an implementation detail, as they are required for (currently) three things: - Vertex buffers (for `Tess`). - Index buffers (still for `Tess`). - And uniform buffers, which are known to be problematic because of the unsafe nature of using them and will require a redesign. So better get rid of the concept of buffers and expose directly the upper abstractions, such as slicing `Tess`, which is already supported and then won’t be a breaking change. The only case that breaks is when people want to build a `Tess` from GPU buffers, which is not going to be supported anymore. Relates to #483.
Buffers are not really needed / used by people and are an implementation detail, as they are required for (currently) three things: - Vertex buffers (for `Tess`). - Index buffers (still for `Tess`). - And uniform buffers, which are known to be problematic because of the unsafe nature of using them and will require a redesign. So better get rid of the concept of buffers and expose directly the upper abstractions, such as slicing `Tess`, which is already supported and then won’t be a breaking change. The only case that breaks is when people want to build a `Tess` from GPU buffers, which is not going to be supported anymore. Relates to #483.
I'm noticing a problem where intermittently, in some of my
Tesses, when their index storage has been updated usingindices_mut(), some of the indices will be seemingly pointing to a vertex with zero data (causing triangles missing or stretched to the world origin):and errors including the following will appear in the Chrome console:
If I modify
luminance-examples-web/src/lib.rsto contain the lineinside
render_scene()(with accompanying changes to have the necessary mutable reference), then I get no rendering glitches but I do see this error and stack:This suggests that
BufferSliceMutWrapper::dropis somehow not correctly managing the GL state, such thatbindBufferfails and the followingbufferSubDatathen overwrites an unrelated buffer from the previous binding.The text was updated successfully, but these errors were encountered: