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

WebGPURenderer: Improve AO approach #28863

Draft
wants to merge 19 commits into
base: dev
Choose a base branch
from
Draft

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Jul 13, 2024

Related issue: #28844

Description

add standardPass() and improve ao approach
The idea here is to have a simplified pass() making available the main and common resources of the renderer, such as ao, bloom, 3dlut, which can be enabled or disabled, leaving the approach part to us, which would simplify mainly for the beginner to have a PP of good quality.

I'll create separate PRs for render.transparent/opaque, NodeHandler and after/before before merge this one.

AO Approch

image

Live example

I will improve this example too 😅. It is possible to see a transparent object and an emissive one respecting the lighting model.

image

Copy link

github-actions bot commented Jul 13, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
683.7 kB (169.3 kB) 683.7 kB (169.3 kB) +0 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
460.9 kB (111.2 kB) 460.9 kB (111.2 kB) +0 B

@@ -2,14 +2,15 @@
import { nodeObject, addNodeElement, tslFn, float, vec4 } from '../shadernode/ShaderNode.js';
import { NodeUpdateType } from '../core/constants.js';
import { uv } from '../accessors/UVNode.js';
import { after } from '../utils/AfterNode.js'

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused import after.
@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 14, 2024

I'm all in for improving quality but we have to make sure to make the examples not too complex. Or at least we should provide a good mix of simple and advanced demos.

Hence, I suggest to keep webgpu_postprocesing_ao as it is and only update the blendIntensity related code. Examples like webgpu_postprocesing_ao, webgpu_postprocesing_dof or webgpu_postprocesing_afterimage should be as simple as possible and just demonstrate the pass and provide a GUI for configuration.

Then we can add advanced examples (webgpu_postprocesing_advanced_*) which demonstrate more complex setups. I suggest you create webgpu_postprocesing_advanced_ao and put the new code there (meaning the one with a separate opaque pass). Are you okay with that?

A component like standardPass is a good idea to simplify the setup for beginners. The FX setup code in webgpu_postprocesing_advanced_ao is something I would like to hide as good as possible, tbh. standardPass could be responsible for setup the pass chain and ensure a correct pass order so users can just add it to their scene and configure the different effects.

That said, it should still be easy to configure a basic pass chain on app level so I would like to not move away from this possibility. But it's a good idea to provide a component like standardPass that helps with more complex setups that might overwhelm the average user.

@@ -20,7 +20,7 @@
<script type="module">

import * as THREE from 'three';
import { pass, mrt, output, transformedNormalView } from 'three/tsl';
import { uniform, pass, standardPass } from 'three/tsl';

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused import pass.
@sunag
Copy link
Collaborator Author

sunag commented Jul 14, 2024

I was coming to this conclusion too, yesterday I made some related modifications and I'm updating the PR. I was thinking about just keeping webgpu_postprocesing_ao using standardPass(), and I added aoIntensityNode because this level of external manipulation wouldn't be possible in standardPass() node.

What do you think about standardPass( scene, camera, { enableAO: true } )?

This should be the code necessary to obtain AO:

postProcessing = new THREE.PostProcessing( renderer );

const scenePass = standardPass( scene, camera, { enableAO: true } );
// scenePass.aoIntensityNode = uniform( 1 ); // optional

postProcessing.outputNode = scenePass;

I thought about the possibility of having an opaquePass that we could share but we would have problems with the AO not respecting the lighting model.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 14, 2024

What do you think about standardPass( scene, camera, { enableAO: true } )?

Yes, it should be possible to enable/disable the effects. But tbh, I'm still unsure about the API at this point and would like to hear the opinion of @mrdoob.

The thing is....if we have something like standardPass, the question is do we add other passes in the future? If there is AO, one could argue to add other techniques which have a similar relevance like Bloom, DOF, Motion Blur and various anti-aliasing. But that would blow up the API and would move away from the pass chain model. Some things have to be integrated like SSAA or TRAA since they usually come before AO.

It reminds me a bit at the old material system from WebGLRenderer (e.g. MeshStandardMaterial). At first sight, it feels like a step backwards. Instead of letting the developer compose a pass chain, we try to provide a monolith with a configurable interface. On the other hand, pass chains might get so complex, that something like standardPass provides a more convenient setup approach.

FX is in general a custom step and pass chains usually look different from app to app. I'm not familiar enough with something like standardPass so I can't tell how well this approach works in production. However, the best way to find out is to add standardPass and see how the developers approach it.

That said, I would only use standardPass in webgpu_postprocessing_standard for now. In webgpu_postprocessing_ao, I would go back to the original approach. The classic AO setup does not produce 100% physically correct results in all conditions but similar things can be said for a lot of techniques (SSR is a good example). The "more correct" result is also more expensive (because of the separate opaque pass) and not every app needs this level of accuracy (and wants to accept the related overhead).

I would consider standardPass as the current convenient method to make the results more plausible. Advanced developers can still setup their own pass chain based on the code in standardPass if they require full access to the pass chain.

@sunag
Copy link
Collaborator Author

sunag commented Jul 14, 2024

MRT also results in other overheads, I don't think this approach would have that much overhead considering the benefits. I can't see an ideal scenario for using AO when it's not compatible with transparent materials outside of our example bubble.

About standardPass I think it wouldn't be ideal either if we had a lot of components, the idea of ​​starting with { enableAO: false } was to have compatibility with previous versions, but that doesn't solve the tree shaking problem.

Maybe the ideal would be to have an ao( scenePass ) as a parameter, and through a shared context we can assemble the necessary MRT or pre-pass.

@sunag
Copy link
Collaborator Author

sunag commented Jul 14, 2024

I'll look for a way to remove standardPass approach...

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 14, 2024

MRT also results in other overheads, I don't think this approach would have that much overhead considering the benefits. I can't see an ideal scenario for using AO when it's not compatible with transparent materials outside of our example bubble.

Could we at least make the the opaque pass configurable so if not wanted it can be disabled? In this case the normal and depth buffer from the beauty pass should be used.

Edit: I'm okay if opaque pass is used by default. But it would be great to have the option for an optimization.

Maybe the ideal would be to have an ao( scenePass ) as a parameter, and through a shared context we can assemble the necessary MRT or pre-pass.

Sounds good!


let camera, scene, renderer, postProcessing, controls, clock, stats, mixer;

let aoPass, denoisePass, blendPassAO, blendPassDenoise, scenePassColor, aoIntensity;

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable blendPassAO.

let camera, scene, renderer, postProcessing, controls, clock, stats, mixer;

let aoPass, denoisePass, blendPassAO, blendPassDenoise, scenePassColor, aoIntensity;

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable blendPassDenoise.

let camera, scene, renderer, postProcessing, controls, clock, stats, mixer;

let aoPass, denoisePass, blendPassAO, blendPassDenoise, scenePassColor, aoIntensity;

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable scenePassColor.
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.

None yet

2 participants