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

three-buffergeometry-to-prwm: support int array buffers #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chelkyl
Copy link

@chelkyl chelkyl commented Nov 28, 2021

The core PRWM supports int buffers so this implementation should too.
Includes adding browserify and uglify-js as dev dependencies and including package-lock to support CI/CD pipelines.

@kchapelier
Copy link
Owner

kchapelier commented Nov 29, 2021

Thank you for the PR, I'll hopefully look into it later today (CET time) but it seems ok at first glance.

@kchapelier
Copy link
Owner

It's more complicated than expected. 😞

My understanding is that until at least March 2020, three.js specified all attributes (even those using with Int/UintXArray) as floats, using gl.vertexAttribPointer. The library only started using the WebGL2-only gl.vertexAttribIPointer method to specify integer attributes later that year, and when using the WebGL1 renderer it still fallback to specifying them as floats. https://github.com/mrdoob/three.js/blob/r135/src/renderers/webgl/WebGLBindingStates.js#L291-L303

It's late so this will have to wait, but I'll probably add an arguments to threeBufferGeometryToPrwm to let the user choose how those attributes should be handled as there is no definitive correct answer in this context. Currently, Three.js will always use its own heuristic to decide how to handle those integer array attributes depending on which renderer is used. So this will only matter in scenario where a BufferGeometry is exported from Three.js and used in a non-Three.js environment.

There is apparently some discussion about this with a PR on the way mrdoob/three.js#21595

@chelkyl
Copy link
Author

chelkyl commented Nov 29, 2021

That makes sense! Thank you for taking a closer look at it.

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.

None yet

2 participants