Skip to content

Conversation

greggman
Copy link
Collaborator

@greggman greggman commented Oct 2, 2024

AFAICT, the spec requires toneMapping period. Even if the browser doesn't support toneMapping.mode = 'extended' it requires it still requires it to exist. It will just be set to 'standard' if the implementation does not support 'extended'.

The only other case is when the canvas has not been configured in which case getConfiguration returns undefined.

Both getConfiguration and toneMapping were added to the spec close enough that any implemetation that supports getConfiguration should be requried to include toneMapping.

AFAICT, the spec requires toneMapping period. Even if the browser
doesn't support toneMapping.mode = 'extended' it requires it still
requires it to exist. It will just be set to `'standard'` if the
implementation does not support `'extended'`.

The only other case is when the canvas has not been configured
in which case `getConfiguration` returns undefined.

Both `getConfiguration` and `toneMapping` were added to the spec
close enough that any implemetation that supports `getConfiguration`
should be requried to include `toneMapping`.
@beaufortfrancois
Copy link
Collaborator

You're right. See #451 (comment)

@beaufortfrancois
Copy link
Collaborator

@kainino0x I believe we can merge this PR. What do you think?

@beaufortfrancois
Copy link
Collaborator

beaufortfrancois commented Oct 18, 2024

I'd like to merge it.

@beaufortfrancois beaufortfrancois merged commit d235cb6 into webgpu:main Oct 18, 2024
1 check passed
@kainino0x
Copy link
Collaborator

Sorry I forgot about this PR because I replied to the spec issue and PR instead.
According to the (open) spec PR the ? is still needed because toneMapping is the thing that won't exist in a browser that doesn't support HDR WebGPU (assuming Safari and Firefox pass through that point before supporting it).
https://github.com/gpuweb/gpuweb/pull/4922/files

@greggman
Copy link
Collaborator Author

According to the (open) spec PR the ? is still needed because toneMapping is the thing that won't exist in a browser that doesn't support HDR WebGPU (assuming Safari and Firefox pass through that point before supporting it).

Can we just require toneMapping? Maybe it's minor but one path is "forgot or didn't know I needed the ? and my app crashes with TypeError: Cannot read properties of undefined" and the other just returns undefined so code checking getConfigure().toneMapping.mode at least doesn't crash.

getConfigure() hasn't shipped to stable so we still have time

@kainino0x
Copy link
Collaborator

Require toneMapping and have toneMapping.mode be the thing that's missing on browsers that don't implement it? Would be fine with me.

@beaufortfrancois
Copy link
Collaborator

getConfigure() hasn't shipped to stable so we still have time

FYI Chrome always returns toneMapping in getConfiguration() if configuration is not null. See https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/webgpu/gpu_canvas_context.cc;l=622;drc=a7b08979892095de2548e193c95e93a01ed8b214

@greggman greggman deleted the fix-toneMapping-check branch October 22, 2024 08:34
@greggman greggman restored the fix-toneMapping-check branch October 22, 2024 08:34
@greggman greggman deleted the fix-toneMapping-check branch September 2, 2025 18:35
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