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(webgl): Update video texture bindings before draw #1853

Merged

Conversation

donmccurdy
Copy link
Collaborator

Restores texture.update() call for video textures in WebGL render pipeline. Removes unused _textureUniforms and _textureIndexCounter properties on WEBGLRenderPipeline class.

Context:

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW The WebGL texture classes have grown into a major mess and I have been working on a big simplification. I'd love to land it before we close v9, I'll try to get the PR ready. Should be more satisfying to make fixes on that code base rather than patching the current code.

@@ -44,8 +44,6 @@ export class WEBGLRenderPipeline extends RenderPipeline {
/** WebGL varyings */
varyings: string[] | null = null;

_textureUniforms: Record<string, any> = {};
_textureIndexCounter: number = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, makes me uncertain, how are we tracking texture indices these days? each pipeline has a limited set of slots to play with and needs to distribute its textures over those?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like _textureIndexCounter was added in #1601 and has been unused since then, _applyBindings() appears to handle indices now. I haven't tested that area yet but hopefully if the glTF rendering is working again, multiple textures are supported!

@donmccurdy
Copy link
Collaborator Author

@ibgreen sounds good, will look forward to trying that! Up to you if this PR should be merged in the meantime or not.

@donmccurdy donmccurdy merged commit 42a1c87 into visgl:master Nov 30, 2023
2 checks passed
@donmccurdy donmccurdy deleted the donmccurdy/webgl-video-texture-update branch November 30, 2023 16:18
@donmccurdy donmccurdy added this to the 9.0.0 milestone Feb 26, 2024
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.

2 participants