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

WebGLRenderer: Check for EXT_color_buffer_float for transmission. #27761

Merged
merged 6 commits into from
Mar 4, 2024
Merged

WebGLRenderer: Check for EXT_color_buffer_float for transmission. #27761

merged 6 commits into from
Mar 4, 2024

Conversation

susiwen8
Copy link
Contributor

@susiwen8 susiwen8 commented Feb 17, 2024

Copy link

github-actions bot commented Feb 17, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
669.5 kB (166.1 kB) 669.5 kB (166.1 kB) +33 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
449.7 kB (108.8 kB) 449.7 kB (108.8 kB) +34 B

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2024

Would you be okay with stalling the PR for a couple of weeks? We will remove WebGL 1 from the renderer soon so the change set of this PR will look much smaller which will make it easier to review.

@susiwen8
Copy link
Contributor Author

@Mugen87 Sure not a problem.

We will remove WebGL 1 from the renderer soon
That means threejs won't support webgl1 soon?

@susiwen8 susiwen8 marked this pull request as draft February 18, 2024 01:32
@gkjohnson
Copy link
Collaborator

That means threejs won't support webgl1 soon?

That's correct. WebGL 1 support will be removed in r163

@susiwen8 susiwen8 marked this pull request as ready for review March 4, 2024 09:37
@susiwen8
Copy link
Contributor Author

susiwen8 commented Mar 4, 2024

@Mugen87 I have removed code for webgl1

@@ -1416,7 +1416,7 @@ class WebGLRenderer {

_transmissionRenderTarget = new WebGLRenderTarget( 1, 1, {
generateMipmaps: true,
type: extensions.has( 'EXT_color_buffer_half_float' ) ? HalfFloatType : UnsignedByteType,
type: ( extensions.has( 'EXT_color_buffer_half_float' ) || extensions.has( 'EXT_color_buffer_float' ) ) ? HalfFloatType : UnsignedByteType,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, there is no system that supports EXT_color_buffer_float but not EXT_color_buffer_half_float. Since the transmission render target only uses HalfFloatType and not FloatType, I'm afraid this addition does not make sense.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confusingly the EXT_color_buffer_float extension may imply that half float types are supported. From the spec:

The following floating-point internal formats become color-renderable: R16F, RG16F, RGBA16F, R32F, RG32F, RGBA32F and R11F_G11F_B10F. A renderbuffer or a texture with a color-renderable internal format can be used as a rendering target by attaching it to a framebuffer object as a color attachment.

If a device reports that EXT_color_buffer_float but for some reason doesn't report EXT_color_buffer_half_float we should still support rendering to half float buffers.

Copy link
Collaborator

@Mugen87 Mugen87 Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then let's merge for the case a device only reports EXT_color_buffer_float support.

@Mugen87 Mugen87 added this to the r163 milestone Mar 4, 2024
@Mugen87 Mugen87 changed the title fix: some feature need webgl extension WebGLRenderer: Check for EXT_color_buffer_float for transmission. Mar 4, 2024
@Mugen87 Mugen87 merged commit 3941fd4 into mrdoob:dev Mar 4, 2024
12 checks passed
@susiwen8 susiwen8 deleted the extensions branch March 4, 2024 11:10
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