Simplify and optimize state tracking #1293
Conversation
| @@ -1350,7 +1350,7 @@ test('WebGL#Transform run (custom parameters)', t => { | |||
| const outTexData = transform.getData({packed: true}); | |||
| t.deepEqual(outTexData, expectedData, `${name} Transform should write correct data into Texture`); | |||
|
|
|||
| t.ok(getParameter(gl2, GL.BLEND) === true, 'Parameters are properly set'); | |||
| t.ok(getParameters(gl2, [GL.BLEND])[GL.BLEND] === true, 'Parameters are properly set'); | |||
There was a problem hiding this comment.
getParameters(gl, key) -> getParameters(gl, key)[key], this is breaking API change. Update getParamters doc and also upgrade guide?
Also why do we have to specify the key twice?
There was a problem hiding this comment.
It's getParameter (singular) to getParameters (plural) which returns an object. getParameter (singular) has been removed.
Docs have already been updated.
There was a problem hiding this comment.
@tsherif The perf gains look promising!
A general comment: Due to the amount of code moved, git's diffing process is not effective here and it is hard to see what changes have actually been made. I would for instance be interested in understanding the performance optimization related changes, and also get a handle on what other detailed changes where made.
Just an idea: In cases like this, when I realize the diffing didn't work, I sometimes go back and split this type of PRs into:
- a first PR that just moves code between files without changing anything, that can be quickly landed.
- then a second PR that now clearly shows what was changed in the old code.
Not a must, but if you did, I'd certainly be happy to take a second look.
| // Reset all parameters to a pure context state | ||
| export function resetParameters(gl) { | ||
| setParameters(gl, getDefaultParameters(gl)); | ||
| setParameters(gl, GL_PARAMETER_DEFAULTS); |
There was a problem hiding this comment.
Keep the comment that explains that the defaults will not be correct for certain values, e.g. those that are initialized based on canvas sizes?
|
|
||
| // frameBuffer not supported: use framebuffer API | ||
| // TODO - is this still true? | ||
| assert(!parameters.frameBuffer); |
There was a problem hiding this comment.
This check can be dropped now. No apps have been using params.frameBuffer with that spelling for a long time.
|
The biggest change, performance-wise was getting rid of this: https://github.com/uber/luma.gl/pull/1293/files#diff-5a444b3148edb50012ab27ede7777f31L62 I'm considering generally disallowing that pattern of using |
setParameters,getParameters,withParameters,resetParameters.CPU performance improvement on the deck.gl stress test is significant: