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

Examples: Update from GammaCorrectionShader to OutputPass #26129

Merged
merged 3 commits into from
Jun 14, 2023

Conversation

donmccurdy
Copy link
Collaborator

No particular hurry, but if this is something we're interested in, I can make the same changes to the remaining examples. Would also be fine with updating usage but leaving GammaCorrectionShader un-deprecated. OutputPass is a very nice usability and performance improvement, @Mugen87. 👍

Related issue:

@WestLangley
Copy link
Collaborator

There is a problem with OutputPass that should be fixed.

The transfer function should only be applied to luminance values that have been properly tone mapped to within transfer function's input range. (Less than 1 currently).

So NoToneMapping and LinearToneMapping are problematic.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 24, 2023

No particular hurry, but if this is something we're interested in, I can make the same changes to the remaining examples.

Great!

I would prefer to not merge the PR before the examples are updated though. The number of examples using GammaCorrectionShader is manageable and I just want to avoid deprecation warnings in the example code so close before the next release.

So NoToneMapping and LinearToneMapping are problematic.

AFAICS, this is not a problem specific to OutputPass. The previous approach also had this issue. How do you suggest to fix it?

@WestLangley
Copy link
Collaborator

AFAICS, this is not a problem specific to OutputPass. The previous approach also had this issue.

Correct.

How do you suggest to fix it?

Ultimately, an appropriate tone mapping function is determined by the viewing device and the viewing environment. Likewise, the OETF is device-specific.

For what to do in the mean time, I'm open to suggestions.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented May 24, 2023

More flexible tone mapping functions are well worth exploring, as is output to Display P3. Both will take some time. In the meantime, some but not all of our tone mapping functions clamp. I don't think it would be appropriate for NoToneMapping or LinearToneMapping functions to clamp. We could do so before or after the OETF, but personally I also feel OK with just the implicit clamp imposed by the HTMLCanvas API.

Aside – "Extended" sRGB values outside [0,1] are sometimes used. I have no interest in supporting this in three.js, except to say that passing values >1 through the sRGB OETF is not always the wrong choice.

@donmccurdy
Copy link
Collaborator Author

Let's start with just updating the examples, not deprecating GammaCorrectionShader quite yet. It appears there are failures in some examples, so I'll move this to draft status.

@donmccurdy donmccurdy changed the title GammaCorrectionShader: Deprecate in favor of OutputPass Examples: Update examples from GammaCorrectionShader to OutputPass May 24, 2023
@donmccurdy donmccurdy changed the title Examples: Update examples from GammaCorrectionShader to OutputPass Examples: Update from GammaCorrectionShader to OutputPass May 24, 2023
@donmccurdy donmccurdy marked this pull request as draft May 24, 2023 20:38
@WestLangley
Copy link
Collaborator

@donmccurdy For now, I think it is appropriate to clamp LinearToneMapping. Clamping is post-exposure-control.

When we support HDR displays, this will need to be revisited.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 12, 2023

Let's start with just updating the examples, not deprecating GammaCorrectionShader quite yet.

Totally agree! Do you think we can move with this PR forward and update at least the example that do not break? I would love to see more usage of OutputPass in the examples for r154 and more users using it in their code.

@donmccurdy donmccurdy marked this pull request as ready for review June 13, 2023 18:12
@donmccurdy donmccurdy force-pushed the chore/deprecate-gammacorrectionshader branch from e3ab0fd to 3c5bf85 Compare June 13, 2023 18:12
@donmccurdy
Copy link
Collaborator Author

@Mugen87 updated, thanks!

@Mugen87 Mugen87 merged commit e539c2d into mrdoob:dev Jun 14, 2023
@donmccurdy donmccurdy deleted the chore/deprecate-gammacorrectionshader branch June 14, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants