Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ThinEngine.updateDynamicIndexBuffer #12564

Merged
merged 5 commits into from
May 25, 2022

Conversation

barroij
Copy link
Contributor

@barroij barroij commented May 23, 2022

This is to fix a bug we ran into.

To repro :

  • at creation time, provide a mesh with indices of type Array<number> => if all the indies are below 65535, the Array<number> is normallized into an UInt16Array in ThinEngine._normalizeIndexData

  • call Mesh.updateIndices with a UInt32Array => instead of converting to UInt16Array as if a Array<number> had been passed, the UInt32Array is forwarded to WebGL, leading to wrong indexing.

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@barroij
Copy link
Contributor Author

barroij commented May 24, 2022

and here is a very simple playground with the bug : https://playground.babylonjs.com/#VKBJN#2738

@barroij
Copy link
Contributor Author

barroij commented May 24, 2022

@sebavan @deltakosh I thought I made a comment on my changes but I cannot see it up there. Did I forget to click the "Comment" button ? 😑

I changes the codes so that the same code is used in WebGPUEngine.updateDynamicIndexBuffer and in ThinEngine.updateDynamicIndexBuffer.
I could have created a function to share the code but I do not know what's the best place for that function.

Also There might be an issue in NativeEngine.updateDynamicIndexBuffer which uses _normalizeIndexData to convert the input IndicesArray into Uint16Array or Uint32Array. This function does not consider the type of the pre-existing IndexBuffer and then could lead to the same kind of error.

As a side note, concerning this function _normalizeIndexData, it goes likes this :

if (this._caps.uintIndices) {
    if (indices instanceof Uint32Array) {
        return indices;
    } else {
        // number[] or Int32Array, check if 32 bit is necessary
        for (let index = 0; index < indices.length; index++) {
            if (indices[index] >= 65535) {
                return new Uint32Array(indices);
            }
        }

        return new Uint16Array(indices);
    }
}

Which does not look like a very good idea too me in case of updatable IndexBuffer, even if it should work in most cases.
Determining the type on the IndexBuffer base on the content the IndicesArray passed at creation time is a bit dangerous, as there is no guarantee that this initial IndicesArray uses all the possible indices that could be used in the future using updateIndices.

@deltakosh
Copy link
Contributor

all good for me besides the formatting (just run 'npm run format:fix')

@barroij
Copy link
Contributor Author

barroij commented May 25, 2022

formatted !

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@deltakosh deltakosh merged commit e90ccb8 into BabylonJS:master May 25, 2022
@barroij barroij deleted the fix_index_buffer branch June 16, 2022 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants