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

Replaced TwoPassDoubleSide with material.forceSinglePass #25239

Merged
merged 10 commits into from
Jan 26, 2023
Merged

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jan 5, 2023

Related issue: #25165 (comment)

Replaced TwoPassDoubleSide with material.forceSinglePass.

@mrdoob mrdoob added this to the r149 milestone Jan 5, 2023
@mrdoob
Copy link
Owner Author

mrdoob commented Jan 5, 2023

Another option I can think of is object.renderForceSinglePass. I'll give that a try...

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 5, 2023

Feels better to me...

@mrdoob mrdoob changed the title Replaced TwoPassDoubleSide with object.renderDoubleSideSinglePass Replaced TwoPassDoubleSide with object.renderForceSinglePass Jan 5, 2023
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 5, 2023

We had many complains that the double pass rendering is the default for transparent double-sided objects. I understand it is useful for viewer applications like model-viewer but users frequently reported massive performance drops in their applications after the double pass rendering was introduced. In almost all cases, that happened because double-sided, transparent objects are often used for rendering flat vegetation like grass where the two-pass system produces no quality gains. Speaking for those users, I would prefer a single pass as the default.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 5, 2023

I think the reason they complained was because there was no way to get the performance back.

The original problem we are solving here is explained in #21967; there were way more reports of people asking why their transparent models looked weird.

I think it's better to prioritise a good experience for new users than saving performance for advanced users (whom can now get it back by changing a boolean).

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 5, 2023

Fair enough! I just hope this is the last time we are changing the API in this context^^.

@WestLangley
Copy link
Collaborator

I think two pass rendering for transparent, double-sided objects should only be performed if the blending is NormalBlending.

For other blending modes, the consequences are difficult to comprehend.

/ping @elalish
/ping @donmccurdy
/ping @gkjohnson

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 5, 2023

Perhaps object.renderForceSinglePassIfDoubleSidedAndNormalBlending? 😅

More seriously — I worry that we're making the API unintuitive here, in the name of making invalid configurations impossible. I don't know whether additive blending makes sense with this mode either, but I think it might be more important to behave predictably than to protect the user from incorrect settings. I would ignore the blending mode, come what may.

It is worth noting, too, that .renderForceSinglePass affects only the two-pass DoubleSide behavior. Other behaviors require additional passes, like material.transmission, and would not be affected. Co-locating the setting with material.side had an advantage in this area, and in discoverability.

That said — I am OK with the .renderForceSinglePass proposal, with or without the NormalBlending check.

@titansoftime
Copy link
Contributor

I have to say, digging through documentation to find a material parameter in order to solve a performance issues is not ideal. Do most people read every line of documentation or am I the odd one lol?

Performance affects all users. Double pass transparency seems to be a need of a more specific use case.

Obviously do whatever you guys think is best, just my 2 cents.

@donmccurdy
Copy link
Collaborator

I am concerned about that as well. If users have a transparency-related problem, reviewing the available material properties is pretty intuitive. If users have a performance problem ... there is really nothing that gets them here, you just have to already know this flag exists.

On the other hand, THREE.DoubleSide is already a drag on performance. People enable it for various reasons (Blender defaults, hiding holes in OBJ models, ...) but ideally it should only be turned on to solve a specific problem. That would be true regardless of this change, so maybe the performance cost is limited enough. 😕

@elalish
Copy link
Contributor

elalish commented Jan 5, 2023

I'm not too picky about the API; honestly I think most people will find the solution not from perusing the three.js docs, but from searching, which will likely lead them to this thread and others that point to the correct fix. In my experience, the folks worried about performance tend to be a lot more technical than the folks worried about display fidelity. For this reason I agree with @mrdoob above that the default should favor the less technical user group, as they are less likely to find the API solution, regardless of where it is. Also note that these discussions strongly favor the opinions of the more technical folks, as the less technical are more intimidated.

@WestLangley
Copy link
Collaborator

/bump

@donmccurdy
Copy link
Collaborator

I don't think that I agree we can characterize performance as a power-user concern, and PBR fidelity as a common-user concern. Perhaps in model-viewer that is true. I think we need to weigh the frequency and severity of the visual problem, the frequency and severity of the performance problem, and the journeys users will take in trying to solve those problems.

As an example – we don't compute MikkTSpace tangents for all models with normal maps, because it imposes a high performance cost on many scenes, while the visual improvement affects very few.

But that said, I still don't know if I have a preference between defaulting this to on or off. The visual problems here seem to be more common than the MikkTSpace-related ones.


One other idea: what if we made this a WebGLRenderer property, instead of Object3D or Material? I realize it means less fine-grained control, but I don't imagine users mucking with this on a per-object basis. The property doesn't make sense in three-gpu-pathtracer anyway, and doesn't have any meaning if OIT is implemented in userland either. Also saves users a traverse on their imported models.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 26, 2023

Having another go at this with fresh eyes made me see it more clearly that the property had to be in the material.

I moved object.renderForceSinglePass to material.forceSinglePass.

It already feels much better in the editor:

Screen.Recording.2023-01-26.at.4.40.31.PM.mov

Educative even.

@mrdoob mrdoob changed the title Replaced TwoPassDoubleSide with object.renderForceSinglePass Replaced TwoPassDoubleSide with material.forceSinglePass Jan 26, 2023
@mrdoob
Copy link
Owner Author

mrdoob commented Jan 26, 2023

Any suggestions for better names than material.forceSinglePass?

@mrdoob mrdoob merged commit fcd98c8 into dev Jan 26, 2023
@mrdoob mrdoob deleted the doubleside branch January 26, 2023 13:04
@WestLangley
Copy link
Collaborator

@mrdoob said

the property had to be in the material.

Can you please explain your reason?

Why not on the mesh or object -- for example, like object.frustumCulled?

@WestLangley
Copy link
Collaborator

WestLangley commented Jan 26, 2023

Any suggestions for better names

.enableTwoPassRendering -- default true.

.enableTwoPassDoubleSideRendering

.enableTwoPassDoubleSided

.enableTwoPassDoubleSide

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 27, 2023

Can you please explain your reason?

This felt good:

const geometry = new THREE.SphereGeometry();
const material = new THREE.MeshStandardMaterial( { opacity: 0.5, transparent: true, forceSinglePass: true } );

const  mesh1 = new THREE.Mesh( geometry, material );
mesh1.position.set( 2, 0, 0 );
scene.add( mesh1 );

const  mesh2 = new THREE.Mesh( geometry, material );
mesh2.position.set( - 2, 0, 0 );
scene.add( mesh2 );

Compared to this:

const geometry = new THREE.SphereGeometry();
const material = new THREE.MeshStandardMaterial( { opacity: 0.5, transparent: true } );

const  mesh1 = new THREE.Mesh( geometry, material );
mesh1.position.set( 2, 0, 0 );
mesh1.renderForceSinglePass = true;
scene.add( mesh1 );

const  mesh2 = new THREE.Mesh( geometry, material );
mesh2.position.set( - 2, 0, 0 );
mesh2.renderForceSinglePass = true;
scene.add( mesh2 );

0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Jan 30, 2023
CodyJasonBennett added a commit to CodyJasonBennett/three.js that referenced this pull request Dec 31, 2023
Removes `TwoPassDoubleSide` in favor of `Material.forceSinglePass` (mrdoob#25239).
Mugen87 pushed a commit that referenced this pull request Dec 31, 2023
Removes `TwoPassDoubleSide` in favor of `Material.forceSinglePass` (#25239).
AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
Removes `TwoPassDoubleSide` in favor of `Material.forceSinglePass` (mrdoob#25239).
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.

7 participants