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

Better handling of ModelExperimental indices in WebGL1 #10340

Closed
j9liu opened this issue May 2, 2022 · 2 comments · Fixed by #10344
Closed

Better handling of ModelExperimental indices in WebGL1 #10340

j9liu opened this issue May 2, 2022 · 2 comments · Fixed by #10344

Comments

@j9liu
Copy link
Contributor

j9liu commented May 2, 2022

This came up with #10332 being merged.

WebGL1 doesn't have the ability to get data from a buffer on the GPU. Therefore, the index buffer must be stored in CPU memory to compute new indices for the wireframe. However, this is unideal for users who won't ever use debugWireframe for their purposes.

There are two possible solutions:

  1. Find a way to store the index buffer's resource loader, and load the index buffer originally on the GPU. If debugWireframe is enabled, then reload this resource as a typed array.
    a. This might complicated to figure out, but it could be possible. Could we store the index resource loader separately from the others in GltfLoader?
    b. This would also only involve changes to the loading system; there'd be no changes to the public API.
  2. Add a debug option or flag to ModelExperimental that the user must specify before the model is loaded. If this flag is true, then load the index buffer as a typed array.
    a. This would simplify the loading logic. It may also be expandable if other debug options need this in the future (though I'm not sure what those would be?)
    b. This would involve changes to the API for both models and tilesets. It also may not be intuitive that having debug: true is necessary for debugWireframe to work, but not for debugShowBoundingVolume.
@lilleyse
Copy link
Contributor

lilleyse commented May 2, 2022

Solution 1 is complicated because we might need to reload the entire glTF if the index buffer was embedded in the glTF.

I prefer option 2. Maybe it's ok if the name is something specific like enableDebugWireframe. Really it's all about the documentation being super clear about what's going on.

@ptrgags
Copy link
Contributor

ptrgags commented May 2, 2022

I think that option 2 makes the most sense here.

I think I like the more specific enableDebugWireframe since debug: true would be confusing.

My only concern is if we need to do other things like this in the future, what's the cleanest convention for the parameters? ModelExperimental/Cesium3DTileset already have a huge number of parameters as it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants