Fix Framebuffer isSupported test

[tsherif]

• Fix WEBGL.color_buffer_float typo
• Fix logic (half float check would overwrite float check)
• Check OES_texture_float to support WebGL 1 contexts that don't report WEBGL_color_buffer_float (e.g. Safari)

[tsherif]

@1chandu Not sure if you were using this method, but this might be helpful for your Transform work.

[coveralls]

Coverage increased (+4.00006%) to 66.599% when pulling 25f5a22 on fix-fb-supported into 6a3d027 on master.

[ibgreen]

Should we build this into our features system instead of creating "a shadow features system" in Framebuffer.isSupported?

Now the WebGL1/2 extensions are a complicated mess so I might miss something here, but I do not quite understand the logic: OES_texture_float represents the ability to read from floating point textures, whereas WEBGL_color_buffer_float and EXT_color_buffer_float represent the ability to write to float point textures.

I could see the value for a function that checked both read and write at one time (though we already provide hasFeatures for that).

However since this code uses || operator, the result will be true if the framebuffer either supports reading, or writing, or both, which does not seem that useful?

[tsherif]

OES_texture_float implicitly enables color_buffer_float, and is the only way it's reported in some implementations (e.g. Safari).

See the last line in the overview section here: https://www.khronos.org/registry/webgl/extensions/OES_texture_float/

[ibgreen]

Ok thanks. I'd personally really appreciate if a comment with that link was added to the source code.

Also, the text is so brief that I can barely understand what they are saying: that this extension implicitly activates the write extension if that one is present...

But if not present it won't be activated so we still need to check for it?

[tsherif]

I believe in most cases where OES_texture_float is available you'll be able to render to float textures, but I have heard cases where that wasn't the case, mostly on older mobile devices. If we want to be 100% sure, we'd have to create a framebuffer with a float render target, try to draw to it, and check for errors. My feeling is this would be overkill, but I have seen some engines do it: https://github.com/playcanvas/engine/blob/9ab0893e6100473df41002094fdf18b7028c1081/src/graphics/device.js#L31

[ibgreen]

I don't think we need to go that far. I would just appreciate a comment with a precise description of why we are doing it this way, e.g.

// On Safari WebGL1, floating point textures are renderable when `OES_texture_float` is supported, even though the `WEBGL_colorbuffer_float` extension is not supported. See comment in standard: ...

Without that I don't think I could maintain this code (i.e. understand why it was written like this).

[ibgreen]

OK I see the comments now, looks good, thanks for bearing with me.

[tsherif]

No problem. Thanks for keeping me honest 😄

[ibgreen]

I think it could be worth some explanation around the fact that these are overlaping WebGL1 and WebGL2 extensions. It is not our mess but I had to read up on all the extensions to review this code.

[tsherif]

Sure. This just simplified things a bit since I have to check several for the float test, and in general, it seems redundant to me to split up the extensions by API version. You can check the extension, and if it's written against a different API version, it just won't be available.

[ibgreen]

Ok the comment lines helps, thanks 👍

[ibgreen]

I don't think we need to go that far. I would just appreciate a comment with a precise description of why we are doing it this way, e.g.

// On Safari WebGL1, floating point textures are renderable when `OES_texture_float` is supported, even though the `WEBGL_colorbuffer_float` extension is not supported. See comment in standard: ...

Without that I don't think I could maintain this code (i.e. understand why it was written like this).

[ibgreen]

Ok the comment lines helps, thanks 👍

[ibgreen]

OK I see the comments now, looks good, thanks for bearing with me.

[1chandu]

Overall looks good to me, but as @ib mentioned, should we incorporate this into FEATURES system? Thats what I am checking in deck.gl for GPGPU features.

Current setup can take an array of extensions, but it returns true only if all of them are supported. We might need additional changes, can be done in a different PR.

[tsherif]

Can definitely look into it, but it would require some changes to how FEATURES is implemented, mainly allowing more than one extension to be checked per version, e.g. https://github.com/uber/luma.gl/blob/master/modules/webgl/src/features/webgl-features-table.js#L73

Also an issue here: https://github.com/uber/luma.gl/blob/master/modules/webgl/src/features/webgl-features-table.js#L73
WebGL 2 can use both those extensions: https://webglstats.com/webgl2/extension/EXT_disjoint_timer_query

[ibgreen]

mainly allowing more than one extension to be checked per version,

I don't recall all the details, but I believe the features table reader function already supports function-valued entries (which should suffice), and maybe also array entries (which would make it more elegant).

WebGL 2 can use both those extensions:

Yup, not surprised, makes total sense to continuously adjust the table to match the current situation.

This is the value of a well-maintained feature table. As your PR indicates, the interpretation and specification of some of these extensions evolves with time. E.g. I did a deep dive into the disjoint-query extension two years ago, but there were open questions and the status quo may well have changed (or perhaps I never got it right in the first place).