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

Material: Add TwoPassDoubleSide. #25165

Merged
merged 2 commits into from
Dec 22, 2022
Merged

Material: Add TwoPassDoubleSide. #25165

merged 2 commits into from
Dec 22, 2022

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Dec 21, 2022

Fixed #24711.
Fixed #25149.

Description

Alternative implementation to #25156 as suggested in #24711 (comment). Instead of introducing a new Object3D property, a new Material.side constant controls the rendering of double-sided transparent objects instead.

@Mugen87 Mugen87 added this to the r148 milestone Dec 21, 2022
@mrdoob mrdoob merged commit 31bcc6d into mrdoob:dev Dec 22, 2022
@0b5vr 0b5vr mentioned this pull request Dec 23, 2022
10 tasks
0b5vr added a commit to three-types/three-ts-types that referenced this pull request Dec 27, 2022
See: mrdoob/three.js#25165

also add doc comment of Side
@elalish
Copy link
Contributor

elalish commented Dec 27, 2022

This sounds good, but is it a regression by default now? I think GLTFImporter sets Material.side = DoubleSide for double-sided materials. Should we update it to set Material.side = TwoPassDoubleSide when BLEND mode is used?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 28, 2022

I don't think I'd consider it a regression, one way or the other. But we can do whichever we think is likely to be the best choice for most users, and I'm fine with either option.

@LeviPesin
Copy link
Contributor

Two-pass rendering feels like something that should be opt-in (as after this PR) rather than opt-out.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 28, 2022

According to the user feedback in the last months I think that, too.

@elalish
Copy link
Contributor

elalish commented Dec 28, 2022

My impression from the user feedback (though I can't find the older threads now) was that the main reason to avoid two-pass rendering was for performance in some non-PBR use cases. For PBR, we get pretty ugly artifacts without the two-pass, hence why it was added. Since GLTF is for PBR, this is why I'd recommend changing this default in the Loader. Especially because this will be a pretty tricky change for a user to make after the fact, and also a difficult artifact to debug in the first place in order to find this solution.

@donmccurdy
Copy link
Collaborator

I don't think I agree that two-pass rendering is more necessary for PBR materials than any other materials, or that the performance cost is any less relevant with PBR. Or that "GLTF is for PBR" for that matter. We can't advise people to use FBX just because they want unlit materials, glTF is also a great choice for that.

Both the visual issue we originally solved here, and the performance regression introduced by our fix, are difficult for users to debug themselves. It is probably most important that we be consistent in whatever we decide is the right default.

@WestLangley
Copy link
Collaborator

I, too, think two-pass rendering for transparent double-sided materials should be the default. @mrdoob also thought so, which is why he added the feature.

IMO, this has nothing to do with glTF or PBR.

I also think users who are rendering single-plane, transparent, double-sided objects and are having a performance issue should have the ability to opt-out.

This means reverting this PR in favor of a different API.

//

Also, IMO, this PR does not "fix" #25149 as claimed in the original post above. The issue with gl_FrontFacing remains. But that is a separate issue.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 28, 2022

This means reverting this PR in favor of a different API.

I don't think I followed the link between the earlier sentences and this conclusion, why do you feel that TwoPassDoubleSide is the wrong API? I do prefer it to having a new property on THREE.Object3D, which will be less intuitive to find there.

@elalish
Copy link
Contributor

elalish commented Dec 28, 2022

Maybe something like SinglePassDoubleSide so that DoubleSide can have the same default behavior as before?

@WestLangley
Copy link
Collaborator

@donmccurdy I thought we would have to revert to an different API, but maybe the suggestion by @elalish is OK...

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 28, 2022

Thanks @elalish and @WestLangley!

If we think that two-pass rendering is the right default for most users then giving single-pass rendering another name makes sense to me. I don't know whether @mrdoob still prefers two passes, as the performance drawbacks were less obvious at the time of the original change.

Aside — There's some advantage to leading with a common prefix when we can. Like DoubleSide and DoubleSideSinglePass. Both names will appear side-by-side in autocomplete and other developer tooling, making the choice explicit and harder to overlook.

@WestLangley
Copy link
Collaborator

Well, DoubleSideSinglePass is better IMO,

...but this is such an edge case, I compare it to frustumCulled = false, and would personally prefer an opt-out at the object level instead of material.side = DoubleSideSinglePass.

joshuaellis added a commit to three-types/three-ts-types that referenced this pull request Jan 3, 2023
* docs: update constructor doc comment of geometries

See: mrdoob/three.js#25086

* docs: update doc comment of the constructor of BufferAttribute

See: mrdoob/three.js#25046

the `normalized` was already optional in the typedef

* feat: add TwoPassDoubleSide

See: mrdoob/three.js#25165

also add doc comment of Side

* feat: add `Mesh.getVertexPosition()`

- add `Mesh.getVertexPosition()`
- add doc comment of `SceneUtils.reduceVertices`

See: mrdoob/three.js#25049

* feat: add `Object3D.getObjectsByProperty()`

- add `Object3D.getObjectsByProperty()`
- add doc comment of `Object3D.getObjectByProperty()`

See: mrdoob/three.js#25006

* docs: add a doc comment of `PointLight.castShadow`

See: mrdoob/three.js#25136

* feat (GLTFLoader): add loadNode hook

See: mrdoob/three.js#25077

* feat (Nodes): New features and revisions

- add three new nodes: `CacheNode`, `StackNode`, and `SpecularMIPLevelNode`
- add `NodeCache`
- add `getDefaultUV()` to `CubeTextureNode` and `TextureNode`
- add `isGlobal()` to `Node`
- add `cache` and `globalCache` to `NodeBuilder`
- add missing `flowsData` to `NodeBuilder`
- add `hasDependencies` to `TempNode`
- add `maxMipLevel` and `cache` to `ShaderNodeBaseElements`
- replace `maxMipLevel` -> `specularMIPLevel` of `ShaderNodeElements`
- change constructor type and texture type of `MaxMipLevelNode`
- add `SpecularMIPLevelNode`

* feat (Nodes): add FogExp2 node

Co-authored-by: Josh <[email protected]>
@WestLangley
Copy link
Collaborator

material.transparent takes two values.

material.side now takes four values.

But all 8 possible combinations do not make sense.

That means there is something wrong with the API.

@WestLangley
Copy link
Collaborator

WestLangley commented Jan 4, 2023

Setting material.side to TwoPassDoubleSide renders single-sided when material.transparent is false.

If that combination makes no sense, then the API is not the right API.

If that combination does make sense, then this PR is buggy.

Screenshot 2023-01-04 at 1 41 55 PM

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 4, 2023

TwoPassDoubleSide is only intended for double-sided, transparent objects.

THB, I see no reason for changing anything since I question your premise that every possible combination of properties has to make sense. Let's just take OrbitControls as an example. Certain properties only makes sense when using perspective cameras. Others are intended for orthographic. That does not mean OrbitControls has a bad API.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 4, 2023

TwoPassDoubleSide is only intended for double-sided, transparent objects.

Is this necessarily true? I can imagine using TwoPassDoubleSide for additive blending with .transparent = false. Not sure if it solves a problem or not, but it doesn't seem unreasonable to allow and so I'm not sure the API needs to make that impossible.

@WestLangley
Copy link
Collaborator

WestLangley commented Jan 4, 2023

TwoPassDoubleSide is only intended for double-sided, transparent objects.

With all due respect, to me, that means the API is a poor one.

I would vote to revert to the previous implementation where two-pass was the default, and add a per-object opt-out akin to .frustumCulled. Opting out is an edge case.

I anticipate DoubleSide and DoubleSideSinglePass (or whatever it is named) will just add confusion for users who will be asking "what's the difference".

@mrdoob
Copy link
Owner

mrdoob commented Jan 5, 2023

Something like this?

if ( material.transparent === true && material.side === DoubleSide && object.renderDoubleSideSinglePass === false ) {

	material.side = BackSide;
	material.needsUpdate = true;
	_this.renderBufferDirect( camera, scene, geometry, material, object, group );
	material.side = FrontSide;
	material.needsUpdate = true;
	_this.renderBufferDirect( camera, scene, geometry, material, object, group );

	material.side = DoubleSide;

} else {

	_this.renderBufferDirect( camera, scene, geometry, material, object, group );

}

#25239

@gkjohnson
Copy link
Collaborator

Fwiw if a material is opaque and set to "TwoPassDoubleSide" I'd expect it to render the opaque object double sided. Rendering the object single sided is clearly misleading.

@mrdoob
Copy link
Owner

mrdoob commented Jan 11, 2023

@gkjohnson Oh! Didn't think of that. Right...

@jtbandes
Copy link

This change should be called out in https://github.com/mrdoob/three.js/wiki/Migration-Guide for r148, since the default behavior for certain objects has changed.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Jan 21, 2023

Added:

Material.side = DoubleSide no longer renders a transparent object in separate front and back passes. Use TwoPassDoubleSide for two-pass transparent rendering.

@d3x0r
Copy link

d3x0r commented Feb 8, 2023

For a few versions now I can't get the back of a solid object to render...
I have a sphere, with 2 halves. The back half is a solid, double sided, non transparent texture. The front is a front-sided transparent texture, so I could see the inside of the ball. This used to work (I don't recall the exact version)
But now, I can't get the back side of the sphere to ever show - if I do 'DoubleSided' or 'FrontSide' it shows, but BackSide or DoubleSide looking at the back side doesn't show. I'm just using .MeshPhongMaterial and some geometry. I'm sort of lazy to simplify this.
I thought maybe this new option would work - but then what, i have to make the back texture also double sided transparent?

@mrdoob
Copy link
Owner

mrdoob commented Feb 8, 2023

@d3x0r Do you mind creating jsfiddle?

@d3x0r
Copy link

d3x0r commented Feb 9, 2023

This is a VERY old version that was barely even working; but is enough to show that it shows the inside-back side of half spheres... This uses three.js 111
https://d3x0r.github.io/three.js-double-side-test/old/index.html

this is the current version (150dev)
https://d3x0r.github.io/three.js-double-side-test/new/index.html

This is the repo...
https://github.com/d3x0r/three.js-double-side-test
phong init (new)
https://github.com/d3x0r/three.js-double-side-test/blob/main/new/ballView.mjs#L860

phong init (old)
https://github.com/d3x0r/three.js-double-side-test/blob/main/old/view.js#L395

I am doing lots of 'work' to build the sphere mesh geometries... but the winding order is correct for the front, so I'd think the back would also be right.
(Though I may have a double-flip and have scaling factors that are (-1,-1,1) too)

I do apologize for not minimizing it to just a plane or something...

@mrdoob
Copy link
Owner

mrdoob commented Feb 9, 2023

@d3x0r Can you please create a minimal jsfiddle that shows the issue? We can't help you otherwise.

@d3x0r
Copy link

d3x0r commented Feb 10, 2023

This should have a 4 pixel image included... without the .map it doesn't not show the backs... But not it's not showing the texture either.

https://jsfiddle.net/ptrceavx/ (105) if ( set , transparent : true ) (like line 173 ) then nothing shows....

https://jsfiddle.net/26dybawe/ (149)

I don't know why the texture isn't showing though- it's all black in the simplified version.
This is why I tried to avoid the fiddle, becaue the answer will be 'no I can't' and then it'll still be 'well sure, we can see on your github.io links that it's not working, but JS fiddle works so it's just you'. It's not.

@mrdoob
Copy link
Owner

mrdoob commented Feb 10, 2023

Is that as simple as you're able to make the example?
Is everything in the code you shared necessary to show the issue you're having?

@mrdoob
Copy link
Owner

mrdoob commented Feb 10, 2023

Here's a simple jsfiddle you can build on top of: https://jsfiddle.net/em4j9hvu/

@d3x0r
Copy link

d3x0r commented Feb 10, 2023

https://jsfiddle.net/86fmkban/2/ added texture - added to window (small red dot upper left, so you can see it IS a valid PNG image) ... it's 2 pixels opaque red and 2 pixels mostly opaque red (slightly transparent)
adding transparent:true or alphaTest:true to the MeshBasicMaterial makes the cube disappear; the cube is black - the default clear color was black - changed it to a dark grey so I can see the black cube. Adding map: to the cube makes it black.

I know I didn't have to - but I did also add a conservative onload so the Image() is definitely loaded... I know images stream in and three.js takes care of that pretty well but... I don't see any reason it should be black.

https://jsfiddle.net/86fmkban/4/
Adding ambient light and some point lights doesn't help (of course it would be black if it was inn the dark?)

@mrdoob
Copy link
Owner

mrdoob commented Feb 10, 2023

Here's your jsfiddle fixed: https://jsfiddle.net/6espjogh/

Please, continue working on the jsfiddle until it shows the issue you're trying to describe.

@d3x0r
Copy link

d3x0r commented Feb 10, 2023

https://jsfiddle.net/7ac2wfh8/3/ seems to work fine still. Includes BufferGeometry, OrthoCamera, phong material, complex geometry -- I see no difference between This and mine other than count of objects; but I still have no backsides. Matched the canvas to be transparent too. swapped the image to the high res one I'm using(worked fine so went back to the small one).
Includes all of the objects; and is copy of the same int code that fails... Thanks for the test - but I actually see 0 difference between this and my code; and the fiddle appears to work fine. (Yes, one object now gets blocked by another, but that's because of the order of the objects in the scene, and that's working right; there's never a time when double side doesn't show back side also. I even updated to use the exact same version of tthree.js the module is using, and it's still failing for me; but it shouldn't be (according to the fiddle with the same code)

@mrdoob
Copy link
Owner

mrdoob commented Feb 11, 2023

Sorry, I can't read blobs of text like that.

I've asked ChatGPT to summarise it:

The author is comparing their code to a working example on jsfiddle (https://jsfiddle.net/7ac2wfh8/3/), but their code is failing despite having no apparent differences. The author has tried various changes, such as making the canvas transparent and swapping the image, but the issue persists. The code in the jsfiddle is a copy of the author's code and should be working, but the author is still encountering problems.

Feel free to ask for help on discord.

@mrdoob
Copy link
Owner

mrdoob commented Feb 11, 2023

Credits to thespite on twitter for the ChatGPT idea:
https://twitter.com/thespite/status/1624206812416299008

@d3x0r
Copy link

d3x0r commented Feb 11, 2023

Credits to thespite on twitter for the ChatGPT idea: https://twitter.com/thespite/status/1624206812416299008

Nice. That IS what I said.

https://jsfiddle.net/ox1bqyse/
Added

 cube.scale.set( -1.1, -1.1, 1.1 );

Anything > 1 for scale causes the back to not show.

@mrdoob
Copy link
Owner

mrdoob commented Feb 11, 2023

You need to add side: THREE.DoubleSide to the material: https://jsfiddle.net/L845hveo/

@mrdoob
Copy link
Owner

mrdoob commented Feb 11, 2023

Nice. That IS what I said.

Maybe you can use ChatGPT to summarise your text before posting here next time.

@sketchpunk
Copy link

I'm having an issue related to this and getting gl_FrontFacing to work right. I setup a simple jsfiddle example :
https://jsfiddle.net/sketchpunk/bnmy0Lv3/19/

WIth transparent:true, both faces get colored red. If I turn transparent:false then uncomment the discard condition in the fragment shader, then both faces get different colors. Not sure if its a depth issue since I'm using a sphere and trying to hide half of it.

For context, I'm trying to recreate minionsart's fake liquid shader from unity.

@WestLangley
Copy link
Collaborator

@sketchpunk For a workaround, see #25149 (comment). It will work for ShaderMaterial; not RawShaderMaterial.

You can also set material.forceSinglePass = true, but depending on your use case, that may cause artifacts we aim to prevent.

If you need more help, please post at the forum.

@d3x0r
Copy link

d3x0r commented Mar 23, 2023

I'm having an issue related to this and getting gl_FrontFacing to work right. I setup a simple jsfiddle example : https://jsfiddle.net/sketchpunk/bnmy0Lv3/19/

WIth transparent:true, both faces get colored red. If I turn transparent:false then uncomment the discard condition in the fragment shader, then both faces get different colors. Not sure if its a depth issue since I'm using a sphere and trying to hide half of it.

For context, I'm trying to recreate minionsart's fake liquid shader from unity.

If you move the ball to y=-0.5 instead of y=0.5 it works (?)

@mrdoob
Copy link
Owner

mrdoob commented Mar 24, 2023

It has been resolved already:
https://twitter.com/SketchpunkLabs/status/1638750725453885440

Repository owner locked as resolved and limited conversation to collaborators Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
10 participants