Update checking support for rendering to float textures #1257
Conversation
| if (status !== gl.FRAMEBUFFER_COMPLETE) { | ||
| throw new Error(_getFrameBufferStatus(status)); | ||
| } | ||
| return this; | ||
| } | ||
|
|
||
| getStatus() { | ||
| const {gl} = this; | ||
| const prevHandle = gl.bindFramebuffer(GL.FRAMEBUFFER, this.handle); |
There was a problem hiding this comment.
bindFramebuffer doesn't return anything. This logic should be removed.
There was a problem hiding this comment.
removed, but still re-binding to null to match the old behavior, we can clean it up in a different PR to avoid any regressions.
There was a problem hiding this comment.
bindFramebuffer doesn't return anything. This logic should be removed.
It is hacky, but the previous value is indeed returned, through the polyfill. It should have been mentioned in comment.
IIRC, it was done like this to avoid fouling up global state.
There was a problem hiding this comment.
I find that very confusing. I don't think the polyfills should be introducing new behaviour.
There was a problem hiding this comment.
restored the old behavior for now, should be addressed separately #1259
| @@ -39,6 +42,26 @@ export const FEATURES = { | |||
| GLSL_TEXTURE_LOD: 'GLSL_TEXTURE_LOD' | |||
| }; | |||
|
|
|||
| // function to test if Float 32 bit format texture can be bound as color attachment | |||
| function checkFloat32ColorAttachment(gl) { | |||
There was a problem hiding this comment.
The result of this check will never change, so store it somewhere.
There was a problem hiding this comment.
there is geFeatures method that will add results at a global scope gl.luma.caps. One solution is have users call getFeatures. But ideally, hasFeature and hasFeatures should also add the result back to this global scope and check only if not already cached.
This PR is fixing a release blocker for 7.3, and hasFeature only called during layer initialization, this can be addressed in a separate PR.
There was a problem hiding this comment.
You can just store it has a global in this file for now. This is an expensive check and shouldn't be repeated.
There was a problem hiding this comment.
Added, more thorough solution : #1258
For #1256
Background
More details in above issue.
Change List