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

Apply fog before tonemapping and encoding #26857

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

Apply fog before tonemapping and encoding #26857

wants to merge 10 commits into from

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Sep 27, 2023

Related issue: #26208

Oletus and others added 6 commits September 25, 2023 15:13
This makes behavior more consistent between rendering to an SRGB texture and rendering to the WebGL default framebuffer. Encoding the color values should always happen after fog has been applied.
@mrdoob mrdoob added this to the r157 milestone Sep 27, 2023
@github-actions
Copy link

github-actions bot commented Sep 27, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
649 kB (160.9 kB) 649.6 kB (160.9 kB) +581 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
442.3 kB (107 kB) 442.9 kB (107.1 kB) +581 B

@mrdoob mrdoob modified the milestones: r157, r158 Sep 27, 2023
@mrdoob
Copy link
Owner Author

mrdoob commented Sep 27, 2023

So yeah... scene.background should be tonemapped too.

Screenshot 2023-09-27 at 22 24 50

@mrdoob
Copy link
Owner Author

mrdoob commented Sep 27, 2023

Refactored WebGLBackground background.isColor code path.

Screenshot 2023-09-27 at 23 02 59

@sunag
Copy link
Collaborator

sunag commented Sep 27, 2023

So yeah... scene.background should be tonemapped too.

This would make my life a lot easier, and this solves one of the problems I'm having when porting transmission materials node-based. To work correctly, the process must work in linear color space while rendering objects and only at the output apply tone mapping and output color space.

@donmccurdy
Copy link
Collaborator

I agree this is the more correct way to do things! But there will unfortunately be users with problems because they need the background to match something in HTML/CSS. I wish we had some way to apply the inverse of the tone mapping to a color, so users can at least compute a fog or background color that will give them the final background color they want.

Many colors in the sRGB gamut cannot be reached by ACES tone mapping outputs though, so this would also limit your choice of background color. Perhaps AgX will be more flexible there, but I'm not sure.

Or perhaps THREE.Fog could take an alpha value?

@sunag
Copy link
Collaborator

sunag commented Sep 27, 2023

I like the idea of rendering with alpha and applying it in Fog too, I think it could solve this issue with HTML/CSS? since the transparent background can provide much more possibilities than a single color. This principle could also follow this reasoning #26857 (comment)

@sunag
Copy link
Collaborator

sunag commented Sep 29, 2023

Some tests using opacity Fog:

image

image

image

@sunag
Copy link
Collaborator

sunag commented Sep 29, 2023

Considering that many people use gradient backgrounds, this would fit perfectly into any HTML/CSS.

three.js.examples.-.Google.Chrome.2023-09-29.14-28-11.mp4

@LeviPesin
Copy link
Contributor

Is this related to #25819?

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 18, 2023

What are the open points in this PR? Do we need add an alpha property to the fog classes?

As mentioned earlier, the status quo is especially confusing when devs use post processing with a scene using fog. As soon as they integrate OutputPass in their pass chain, the scene looks suddenly different compared to the inline tone mapping and color space conversion. Most devs initially think FX is buggy although it actually renders the scene correctly (see #26954 (comment)).

@mrdoob
Copy link
Owner Author

mrdoob commented Oct 19, 2023

I'm unable to find the discussion right now... but @elalish was recently dealing with some browsers not doing alpha compositing properly.

That's why I'm not sure about the transparent fog approach.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Oct 21, 2023

As soon as they integrate OutputPass in their pass chain, the scene looks suddenly different compared to the inline tone mapping and color space conversion.

Perhaps related ... when using post-processing: should fog be part of OutputPass, or another effect pass, rather than inline?

@mrdoob mrdoob modified the milestones: r158, r159 Oct 27, 2023
@mrdoob mrdoob modified the milestones: r159, r160 Nov 30, 2023
@mrdoob mrdoob modified the milestones: r160, r161 Dec 22, 2023
@mrdoob mrdoob modified the milestones: r161, r162 Jan 31, 2024
@mrdoob mrdoob modified the milestones: r162, r163 Feb 29, 2024
@donmccurdy
Copy link
Collaborator

Perhaps related ... when using post-processing: should fog be part of OutputPass, or another effect pass, rather than inline?

Answering my own question... I think I'd prefer not putting fog into OutputPass.

@donmccurdy
Copy link
Collaborator

@mrdoob mrdoob modified the milestones: r163, r164 Mar 29, 2024
@mrdoob mrdoob modified the milestones: r164, r165 Apr 25, 2024
@mrdoob mrdoob modified the milestones: r165, r166 May 31, 2024
@mrdoob mrdoob modified the milestones: r166, r167 Jun 28, 2024
@mrdoob mrdoob modified the milestones: r167, r168 Jul 25, 2024
@mrdoob mrdoob modified the milestones: r168, r169 Aug 30, 2024
@mrdoob mrdoob modified the milestones: r169, r170 Sep 26, 2024
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.

6 participants