Closed zackgomez closed 1 year ago
zackgomez
commented
1 year ago I included a test, and added sinon to be able to spy on the gl context and verify that the caching was working. Don't feel strongly about you including that library but I wanted to test the code and show it works.
greggman
commented
1 year ago yes, I think this is out of scope for twgl.
If you want perf you should probably look into using uniform blocks and separating uniforms in per model, per material, global (camera, projection), etc...
Of course that wouldn't totally eliminate the benefits of a cache since you still have to set sampler locations
On Wed, Nov 30, 2022 at 19:27 Zack Gomez @.***> wrote:
I included a test, and added sinon to be able to spy on the gl context and verify that the caching was working. Don't feel strongly about you including that library but I wanted to test the code and show it works.
— Reply to this email directly, view it on GitHub https://github.com/greggman/twgl.js/pull/219#issuecomment-1333113546, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZKNCVSNLFBC2W65V4VCTWLALJPANCNFSM6AAAAAASQJ7K4A . You are receiving this because you are subscribed to this thread.Message ID: @.***>
zackgomez
commented
1 year ago Yeah I do use uniform blocks for most things but as you say samplers and other "mesh specific" configuration like say alpha or various lighting effects don't use a block.
I'm closing this as it is out of scope.
I view this change right on the border of what twgl does and doesn't want to do. Feel free to reject it without digging into the merits if it is outside the scope of what you want for twgl. Regardless, thank you for the library as both a useful tool and example code.
The reason I'm even making a pull request is that a) i wrote the code b) it is a substantial performance improvement c) it's simple and easy to maintain correctness (assuming the twgl user only sets uniforms via twgl setters).
The specifics here are that each uniform setter has a cache initialized to the default value (arrays could be slightly improved by being initialized to the correct size), and we check the cache before calling each uniform setter and update gl and the cache if necessary.
The failure cases here are a) the user changes uniforms with bare opengl, rendering the cache incorrect. This is a new error case introduced by this change. b) the user changes uniforms with the wrong program mounted, rendering the cache incorrect. This is an error prior to this change. c) perhaps something with losing the webgl context, or some other more global condition- I'm not terribly familiar with the all of the esoteric things that can happen.
As for the performance impact, I have some random scene where I am calling twgl.setUniforms with the full set of uniforms for each draw/shader. It's nice because it's declarative.
The uniform1i (texture unit, boolean settings) are nearly eliminated, the uniform1f calls are fully eliminated.
I am going to build an abstraction to also cache the texture/sampler/blend/enable remaining global opengl state after this, but that change will be much more involved and likely would break the scope of twgl as it would create a wrapper instead of simply reducing verbosity.