Apply fog before tonemapping and encoding

[Oletus]

Related issue: #24362

Description

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.

This preserves the same fog color as before in case ColorManagement.enabled is true. Some example screenshots still need to be updated, since changing the order of operations does affect the rendering results.

If color management is disabled, then the fog color that used to be interpreted as SRGB is now interpreted as linear, which does cause significant changes in render output.

This contribution is funded by Higharc

[Oletus]

Alternatively we could do this to more closely preserve the previous behavior when ColorManagement.enabled === false, though it's kind of unintuitive to apply color management operations in case color management is disabled:

	function refreshFogUniforms( uniforms, fog ) {

		if ( ColorManagement.enabled ) {

			fog.color.getRGB( uniforms.fogColor.value, LinearSRGBColorSpace );

		} else {

			if ( getUnlitUniformColorSpace( renderer ) === SRGBColorSpace ) {

				_color.copySRGBToLinear( fog.color );
				_color.getRGB( uniforms.fogColor.value );

			} else {

				fog.color.getRGB( uniforms.fogColor.value );

			}

		}

		...

	}

What do you think?

[donmccurdy]

The concerns I mentioned in #24362 (comment) have been addressed by updates to color management in r152, so I agree this is a change we should could make.

Still thinking about what the behavior should be if color management is disabled...

[donmccurdy]

Nit: The default value (THREE.ColorManagement.workingColorSpace) would be the best choice. There's a mistake in the Color#getRGB documentation, to be fixed by #26210.

[mrdoob]

@Oletus has this been resolved?

[WestLangley]

Related history: #11076 (comment).

[donmccurdy]

Hmm... I can certainly see an argument that fog should be applied after tone mapping, as suggested by @bhouston in #11076 (comment). Would that still fix #24362? Otherwise fog will need large adjustments for tone mapping exposure.

[LeviPesin]

Fog right now is assumed to be in output space (whatever that is) because often people clear a buffer with a color and then set that color to be the fog color and expect their render to blend to match that clear color. Thus fog should be in "output space" until we support auto-tone mapping the clear colors. Fog needs to be after tone mapping and encoding.

Maybe... we can assume that the color passed to renderer.clearColor is in linear-sRGB (and convert it to renderer.outputColorSpace) and not renderer.outputColorSpace? This will solve the concern above.

[donmccurdy]

@LeviPesin the quote above predates #23937, and that is effectively solved now. I think the remaining concerns are:

1. Whether we want a breaking change for non-color-managed scenes, or an awkward workaround
2. Whether fog should be applied before or after tone mapping

[WestLangley]

Whether fog should be applied before or after tone mapping

I think there is no question as to the "proper" workflow. See. #24362 (comment).

I think we should try to find a way to get there.

[donmccurdy]

If we were discussing lit volumetric fog, I'd have no hesitation in agreeing.

As it is, the way looks problematic. Users often match their fog to the color of the page background or other HTML/CSS elements. Maintaining this behavior would suggest that users should (a) input a fog color that tone-maps to the desired output color, or (b) we should compute that for them. Neither is consistently possible, because tone mapping (and especially ACES) is not capable of generating many possible output colors in the sRGB gamut. This assumption is based on results from @elalish, which I'd love to have misunderstood!

We could ask users to do the reverse, and choose their HTML colors based on the tone-mapped fog color, but it's a limiting requirement and I'm not sure what benefit we're getting from it.

I would be happy to consider moving fog before encoding, as a stepping-stone toward getting it in front of tone-mapping, as well.. 😅

[bhouston]

Is there a way that fog could just mostly modifying alpha? So instead of trying to match the colors of the page in a 3D render, we just fade away the canvas and let the underlying page colors show through? This is more flexible in that you can have more complex things happening in the background that just a color, it could be a wallpaper or gradients, etc.

[elalish]

I don't know much about fog, but I like @bhouston's idea on alpha. That feels very related to the changes I made to transmissive materials on a transparent canvas - not saying it's perfect, but it makes it much easier to be cohesive with page CSS. I'm not familiar with how fog is used though, so I'm not the best person to ask.

[bhouston]

I think real fog should be before tone mapping (yes it contradicts what I wrote years ago) but that is because fog for the purpose of blending into the background should be focused on alpha rather than color. So there are two separate use cases and workflows. Not sure how best to implement it.

-ben

On Wed, Jun 7, 2023 at 3:34 PM Emmett Lalish ***@***.***> wrote: I don't know much about fog, but I like @bhouston <https://github.com/bhouston>'s idea on alpha. That feels very related to the changes I made to transmissive materials on a transparent canvas - not saying it's perfect, but it makes it much easier to be cohesive with page CSS. I'm not familiar with how fog is used though, so I'm not the best person to ask. — Reply to this email directly, view it on GitHub <#26208 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEPV7IBG7ZV7YLP27JMD7LXKDJV5ANCNFSM6AAAAAAY5VVMSE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Ben Houston, CTO https://threekit.com 613-762-4113 / http://calendly.com/benhouston3d

[Oletus]

I'd lean on applying fog after tone mapping after reading the discussion here. It's a smaller step and we don't have a way to map the fog color to the pre-tonemapped space. The important thing is to apply fog before encoding.

It would also be possible to blend the render results to a background buffer by the fog factor, but I'd say that's out of scope for this fix. That seems more like a different, complementary option to the current fog rather than a replacement. It would generate very different results in some cases and whether those results are desirable is a question of preference. It would also have very different performance characteristics, like doubling the required fillrate.

[mrdoob]

It sounds weird to me to do fog after tonemapping.
Aren't we applying tonemapping to scene.background = color too?

[Oletus]

I think the solid color background is implemented as a clear, so it's not subject to tonemapping.

[bhouston]

@Oletus wrote:

I think the solid color background is implemented as a clear, so it's not subject to tonemapping.

This is only because of the single pass with tone mapping that has been the default Three.js way for years. This isn't fully correct, it was just a way to do HDR tone mapping in a single shader pass so you didn't need, which at the time was poorly supported, FP16 or FP32 intermediate buffers.

The correct way to clear would be to clear your FP12/FP32 linear color buffer with the background color, then render everything into it as linear and then as a separate pass, do tone mapping + output color space conversion to the screen. The clear color in this case would be tone mapped and if you did fog during the rendering of the 3D scene to the linear FP16/FP32 buffer, then it would also be tone mapped.

I think the solution, is to support alpha background colors and also a special fog that fades to transparent. And also do standard scene fog prior to tone mapping. This lets you incorporate a scene into a webpage seamlessly and better than if you try to match the clear color + fog color to the background.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev	Filesize PR	Diff
649 kB (160.9 kB)	649.1 kB (160.9 kB)	+137 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.5 kB (107.1 kB)	+135 B

[Oletus]

I rebased the PR on top of dev to fix conflicts. I also changed it so that it applies fog after tone mapping, so that the change doesn't have as significant downsides. I agree that applying fog before tone mapping would be more correct, but not having precise control over the final fog output color is a concern.

It could also make sense to offer blending with the page background as an alternative, but I wouldn't want to increase the scope of this PR too much. I opened #26239 for a more focused discussion of the background blending. I hope we can move forward with this change just to fix the mismatch between rendering to SRGB FBOs and the default framebuffer.

Can I also get feedback on this part?

1. Whether we want a breaking change for non-color-managed scenes, or an awkward workaround

[Oletus]

@bhouston @donmccurdy @WestLangley What do you think?

1. Is the current patch ok or should we add some kind of support for blending with the page background first? See Depth-based fog transparency effect to blend with the page background #26239

2. Should I include the workaround for non-color-managed scenes?

Framebuffer-dependent ordering of fog and SRGB conversion is a clear bug and required an awkward search-and-replace workaround on our end, so I'd really appreciate getting some kind of fix merged.

[Oletus]

Sorry about the long delay, but I rebased the patch and added the workaround for non-color-managed scenes to keep the behavior similar to before - I think this is a good approach. Do you think that this bug fix could be merged?

[Oletus]

I tried re-running make-screenshot a few times, also with increased time in puppeteer.js, but some of the failing screenshots don't seem to be updating. Maybe there are some platform differences (I'm on Windows 10)? So I might need help with those.

[mrdoob]

@Oletus Any chance you could update the PR?

[Oletus]

I'll try to get around to updating the PR tomorrow! Thanks for getting back to this!

[Oletus]

Unfortunately I didn't get to updating the PR today, it'll have to wait until next week!

[Oletus]

I updated the PR, but npm make-screenshot still seems to generate different results for me. I'm running Windows 10 + NVIDIA GTX 1080, driver version 536.99. Suggestions?

[WestLangley]

The order should definitely be fog > tonemapping > colorspace.

Just one more thing...

// Fog code snippet for reference

gl_FragColor.rgb = mix( gl_FragColor.rgb, fogColor, fogFactor );

// Current

#include <tonemapping_fragment>
#include <colorspace_fragment>
#include <fog_fragment>

Currently, tone mapping and color space conversion precede the application of fog, and the input gl_FragColor.rgb and fogColor are both in display-refereed color space, and take values in [ 0, 1 ].

// Proposed

#include <fog_fragment>
#include <tonemapping_fragment>
#include <colorspace_fragment>

By applying fog prior to tone mapping and color space conversion, the input gl_FragColor.rgb is in scene-referred color space, has units of cd/m2, and is unbounded, while fogColor is in display-refereed color space and takes values in [ 0, 1 ].

Mixing two values having different units is a red-flag for me.

Perhaps fogColor would need to be redefined as a scene-referred color in the linear, working color space...

[donmccurdy]

Other than #24362 (fog color in THREE.Reflector), what use cases are we hoping will benefit from this change? Transmission might be another?

The change in color space of fogColor could be applied in UniformsUtils ...

three.js/src/renderers/shaders/UniformsUtils.js

Lines 87 to 98 in f9e5848

	export function getUnlitUniformColorSpace( renderer ) {
	if ( renderer.getRenderTarget() === null ) {
	// https://github.com/mrdoob/three.js/pull/23937#issuecomment-1111067398
	return renderer.outputColorSpace;
	}
	return ColorManagement.workingColorSpace;
	}

From a user perspective, I think the effects proposed here would be:

1. fog is now affected by tone mapping (breaking change, mixed positive/negative effects)
2. fog color is managed consistently across default pipeline, transmission, render targets, and post processing workflows (positive)
3. (TBD) fog can now subtract from drawing buffer alpha (possible workaround for users negatively impacted by (1), and useful in its own right)

[mrdoob]

@WestLangley

Perhaps fogColor would need to be redefined as a scene-referred color in the linear, working color space...

I think it should yes.

[mrdoob]

@donmccurdy

1. fog is now affected by tone mapping (breaking change, mixed positive/negative effects)

Definitely a breaking change but doesn't seem too big on an issue for me compared to all the texture and lighting changes we did last year.