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

Minimize glActiveTexture calls #24492

Merged
merged 3 commits into from
Sep 13, 2022
Merged

Conversation

snagy
Copy link
Contributor

@snagy snagy commented Aug 12, 2022

Description

When running spector.js captures of a complex scene, I noticed a number of calls to glActiveTexture that were followed immediately by a separate call to glActiveTexture. This change checks to see if a texture will be bound (or uploaded) before calling ActiveTexture, improving performance by removing these useless gl calls.

This contribution is funded by Meta

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 15, 2022

This is an interesting PR, thanks! Do you think it's possible to reproduce the redundant gl calls in a small live example? I've prepared a fiddle with a simple textured mesh. Do you think you can modify/enhance it so we have something to test?

https://jsfiddle.net/ftn1xh4b/

@snagy
Copy link
Contributor Author

snagy commented Aug 15, 2022

@Mugen87 The CSM example shows the issue pretty well - we're using CSM so that's where I initially saw it.

Without this change it calls ActiveTexture 4 times (for the 4 shadow sections) when it changes material:
Screen Shot 2022-08-15 at 10 27 00 AM

And after applying this change it doesn't call it at all because the textures were already bound to that slot:
Screen Shot 2022-08-15 at 10 32 18 AM

Because this example only uses 2 materials, you don't see a ton of savings, but on our scenes with 80+ material changes the gl calls add up to a significant chunk of cpu time doing nothing.

@mrdoob
Copy link
Owner

mrdoob commented Aug 17, 2022

Nice!

Is is worth having both state.bindTexture and state.bindTextureToSlot though?

Would it make sense to update all the current calls to state.bindTexture to state.bindTextureToSlot and then delete state.bindTexture and rename state.bindTextureToSlot to state.bindTexture?

@snagy
Copy link
Contributor Author

snagy commented Aug 17, 2022

Nice!

Is is worth having both state.bindTexture and state.bindTextureToSlot though?

Would it make sense to update all the current calls to state.bindTexture to state.bindTextureToSlot and then delete state.bindTexture and rename state.bindTextureToSlot to state.bindTexture?

We could definitely do that. The remaining usage of bindTexture is all around render target stuff (generating mipmaps, setting up framebuffer textures) that doesn't currently do any slot management. The remaining question is whether calling it with slot == undefined should set the slot to currentTextureSlot, or to the last slot. currentTextureSlot would minimize activeTexture calls and match the existing behavior the closest. Using the last slot could potentially keep from rebinding over textures, but that's more content dependent.

@mrdoob
Copy link
Owner

mrdoob commented Aug 17, 2022

So could it be this instead and make it work for both?

function bindTexture( webglType, webglTexture, webglSlot ) {

@mrdoob mrdoob modified the milestones: r144, r145 Aug 31, 2022
@mrdoob
Copy link
Owner

mrdoob commented Sep 13, 2022

Much nicer!

@mrdoob mrdoob merged commit fa6899a into mrdoob:dev Sep 13, 2022
@mrdoob
Copy link
Owner

mrdoob commented Sep 13, 2022

Thanks!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Don't set activetexture for textures that won't be bound

* Fix texture state update of a previously bound texture

* Combine bindTexture and bindTextureToSlot into a single function
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