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

WebGLRenderer: Render transparent doublesided in two calls #21967

Merged
merged 3 commits into from
Jun 9, 2021
Merged

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jun 9, 2021

Description

Another benefit of being able to have multiple programs per material is that we can now render doublesided materials in two calls (back side first and then front side).

This produces less confusing renders.

Before:

Screenshot 2021-06-09 at 12 01 00

After (not perfect, but way less confusing):

Screenshot 2021-06-09 at 12 01 32

At the moment the user has to duplicate meshes to achieve this.

The transmission example does it:
https://threejs.org/examples/webgl_materials_physical_transmission.html

With this change we'd be able to simplify shaders a little bit too:

Screenshot 2021-06-09 at 12 11 33

/cc @WestLangley @Mugen87 @elalish

@mrdoob mrdoob added this to the r130 milestone Jun 9, 2021
@WestLangley
Copy link
Collaborator

This approach superior to setting .depthWrite to false in the transparent, double-sided case.

It just results in two draw calls instead of a single one.

Rendering twice should not be required in the opaque case.

@makc
Copy link
Contributor

makc commented Jun 9, 2021

should not be required in the opaque case

it should not, but unfortunately you have these https://github.com/mrdoob/three.js/issues?q=is%3Aissue+gl_FrontFacing

@takahirox
Copy link
Collaborator

Render calls doubles for double side materials. Wondering the performance impact...

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 9, 2021

Yeah, there will be a performance impact in certain scenarios. However, the renderer will produce better results with transparent double-sided materials. It's some sort of trade-off. If we want to prioritize rendering quality over performance, this PR should be merged.

Rendering twice should not be required in the opaque case.

Maybe doing this for now?

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

@mrdoob
Copy link
Owner Author

mrdoob commented Jun 9, 2021

@WestLangley

Rendering twice should not be required in the opaque case.

Yeah... I was pretty eager to remove all this complexity but I agree, it's better to only do this for transparent materials.

@makc

it should not, but unfortunately you have these https://github.com/mrdoob/three.js/issues?q=is%3Aissue+gl_FrontFacing

These were solved with the workaround in #21205.

@Mugen87

Maybe doing this for now?

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

Yep!

@mrdoob mrdoob merged commit e393f5d into dev Jun 9, 2021
@mrdoob mrdoob deleted the doublesided branch June 9, 2021 17:04
@takahirox
Copy link
Collaborator

takahirox commented Jun 9, 2021

Three.js is primary designed for web 3d. Low-end devices can access the Three.js application. For such devices, the application authors or users may want to prioritize the performance over the visual quality. Actually I as a Mozilla Hubs team member would like to choose the performance for low-end devices.

I also know there are users who wants better quality. So ideally there is an option to choose which.

(But if we do only for transparent double side materials, it might be acceptable?)

Personally I like having the option, but I don't have the performance impact numbers and how much popular transparent + double side materials are now so I'm not really sure whether I should convince or not.

@mrdoob
Copy link
Owner Author

mrdoob commented Jun 9, 2021

@takahirox Yeah, I would be very curious to see a use case that gets affected by this 🤔

@mrdoob mrdoob changed the title WebGLRenderer: Render doublesided in two calls WebGLRenderer: Render transparent doublesided in two calls Jun 9, 2021
@lojjic
Copy link
Contributor

lojjic commented Jun 9, 2021

Any planar geometry using transparency that can be viewed from both sides, I think, will incur double draw calls with no possible benefit. I know I personally have many UI/text/image elements that fit that scenario, including every troika-three-text object using its default material (I could change that default but there are legit cases for seeing text from behind).

It's a nice enhancement for those cases where it makes a difference but there are plenty where it doesn't, so being able to prevent the double draw is pretty necessary IMO. I'd vote for opt-in personally.

@makc
Copy link
Contributor

makc commented Jun 9, 2021

@mrdoob

These were solved with the workaround in #21205.

as if )
#21205 "Bring gl_FrontFacing back" - "back" here implies that before it was removed for a reason, and it did not go anywhere. there are GPUs out there where gl_FrontFacing just does not work

@mrdoob
Copy link
Owner Author

mrdoob commented Jun 10, 2021

@makc

#21205 "Bring gl_FrontFacing back" - "back" here implies that before it was removed for a reason, and it did not go anywhere. there are GPUs out there where gl_FrontFacing just does not work

No... I tried to solve in a different way and then I learnt that the issues was that some GPU drivers do not support gl_FrontFacing inside functions. So I brought gl_FrontFacing back and made sure we didn't use it inside functions.

@mrdoob
Copy link
Owner Author

mrdoob commented Jun 10, 2021

@lojjic

Any planar geometry using transparency that can be viewed from both sides, I think, will incur double draw calls with no possible benefit.

Indeed. I can see that use case. Before adding more API surface would be good if we can investigate if the performance impact is worth it though.

It's a nice enhancement for those cases where it makes a difference but there are plenty where it doesn't

Plenty... So far I'm only aware of 1 use case.

@elalish
Copy link
Contributor

elalish commented Jun 10, 2021

Thank you @mrdoob, this is fantastic! We have been doing this manually in model-viewer for some time and this will let us simplify the code considerably. Regarding performance, we don't have clear numbers to show either, but no one has complained. I certainly haven't noticed a big problem; keep in mind that yes, it's an extra draw call, but it's no more fragments being shaded. When you're performance limited in PBR, it's usually because of too many fragments, as that's where the expensive shaders are. And as for quality; the artifacts are so glaring without this approach that I think it's well worth a little performance hit (the same logic we use in the rest of the PBR shaders).

@lojjic
Copy link
Contributor

lojjic commented Jun 10, 2021

@mrdoob

Plenty... So far I'm only aware of 1 use case.

😆 Haha ok I'll give you that, if you consider all uses of planes a single use case. I still think there are "plenty" of scenarios in which you'd want to use planar geometries that way.

would be good if we can investigate if the performance impact is worth it

I'll attempt to get you some hard numbers for one of my scenes. It's in XR, where I'm already highly draw call limited, and any draw call is actually 2, so my gut says this should be measurable but that's worthless without data to prove it of course.

@arpu
Copy link

arpu commented Jul 6, 2021

with this change have some trouble on many Textures on a Planes DoubleSided and transparent (images) would be good if i can disable the double drawcall in this Scenario

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 7, 2021

@arpu any chance you can share screenshots?

@arpu
Copy link

arpu commented Jul 7, 2021

not sure what screenshot you request @mrdoob

workflow is user can upload images to vrland on server side they are packed in a glb with a plane doubeSide and transparent true
( this allows adding transparent pngs )
Bildschirmfoto von 2021-07-07 19-44-43

screenshot from https://vrland.io/2xQTLN/cypher-chk

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 9, 2021

not sure what screenshot you request @mrdoob

I'm just curious to see what you mean with "trouble" here:

with this change have some trouble on many Textures on a Planes DoubleSided and transparent (images)

@makc
Copy link
Contributor

makc commented Jul 10, 2021

a glb with a plane

thats the same as lojjic issue above

@arpu
Copy link

arpu commented Jul 10, 2021

@mrdoob hope this makes it more clear

threejs 130
Bildschirmfoto von 2021-07-10 14-50-14

threejs 129
Bildschirmfoto von 2021-07-10 14-54-54

the difference in drawcalls with no visual benefits

@KiborgMaster
Copy link

And your decision
http://prntscr.com/1iaqow6

You need to set depthWrite to true in this case.

http://prntscr.com/1ibwhup
This is understandable, but it is not a solution to the problem, because the textures look bad, the edges are ragged.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 2, 2021

It's true that with the current alphaTest implementation, it's not possible to avoid aliasing. Besides when using alphaTest for far distant objects the result can look horrible since geometries start to disappear, see #20522 and #14091.

Hence, it's a valid use case to render grass as transparent objects for better quality.

@WestLangley
Copy link
Collaborator

@KiborgMaster You have 250 draw calls per frame?

@KiborgMaster
Copy link

@KiborgMaster You have 250 draw calls per frame?

Yes on build 129

@KiborgMaster
Copy link

KiborgMaster commented Aug 2, 2021

In general, for those who are looking for a solution, while the developers will decide how best to make it manageable, here is my solution, you can invent the property of the object yourself, or even comment out or remove the code, everyone will decide for himself how it is more convenient for him to do it.

String 19259 i add object.userData.hasOwnProperty('twoCalls')

image

@KiborgMaster
Copy link

That is, you just need to come up with some kind of property and, due to it, activate this processing option if someone needs it.

@WestLangley
Copy link
Collaborator

@KiborgMaster You have 250 draw calls per frame?

Yes on build 129

Use instancing or something.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 2, 2021

Yeah, the new implementation for rendering doublesided transparent objects makes it even more important to use concepts like instanced rendering or batching to improve performance.

@KiborgMaster
Copy link

By the way, based on the code, this is not even a double but a triple processing, which is why the performance collapsed.

image

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 2, 2021

You have not modified your code correctly. The official build looks like so:

if ( material.transparent === true && material.side === DoubleSide ) {
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 );
}

@KiborgMaster
Copy link

KiborgMaster commented Aug 2, 2021

material.side === DoubleSide

And yes, exactly, I accidentally duplicated data, my mistake.

@makc
Copy link
Contributor

makc commented Aug 2, 2021

@KiborgMaster funny how you had to switch to solid grey floor texture to demonstrate aliasing issue, where your original screenshos had this:
Screen Shot 2021-08-02 at 19 09 04

but I agree that

this should be a separate property of the material.

for the sake of keeping everyone happy

@titansoftime
Copy link
Contributor

titansoftime commented Aug 2, 2021

@titansoftime Do you mind comparing the number of draw calls with r128 and >= r130 in your application?

Ok so draw calls are exactly the same in the scene I was originally testing so I suppose this PR is not the culprit. Perhaps #22244 will resolve my issue.

Edit* Yup, it was #22244

@lojjic
Copy link
Contributor

lojjic commented Aug 4, 2021

I'll attempt to get you some hard numbers for one of my scenes

Sorry for the very long delay, I went through a job change and no longer had access to my XR scenes. Instead I've created this simplified test case: https://gyprf.csb.app/

The scene is 25 troika-three-text instances - that's a modest number you could easily reach when building a simple UI/dataviz. I tested it twice, only changing the three version between 0.129.0 and 0.130.1. I also repeated the test using both DoubleSide and FrontSide, to prove that the differences were only due to the sidedness and nothing else.

The timings below are an average-ish measurement from Chrome devtools profiler for the WebGLRenderer.renderObjects method. It's not the most accurate way to measure but it's good enough to show a non-imaginary performance hit due to the double draw calls:

Desktop: Ryzen 3700X, NVidia RTX 2070

Three.js version DoubleSide FrontSide
0.129.0 0.18 ms 0.15 ms
0.130.1 0.8 ms 0.15 ms

Phone: Pixel 5

Three.js version DoubleSide FrontSide
0.129.0 0.9 ms 0.8 ms
0.130.1 2.5 ms 0.8 ms

@Samsy
Copy link
Contributor

Samsy commented Aug 4, 2021

Is there a way to disable this behavior and let the user choose ?
Even if the double render is per default and giving experienced users the possibility to toggle this off.
This is multiplying draw calls on transparent double side objects , very counter performant on well managed applications tbh

@KiborgMaster
Copy link

Is there a way to disable this behavior and let the user choose ?
Even if the double render is per default and giving experienced users the possibility to toggle this off.
This is multiplying draw calls on transparent double side objects , very counter performant on well managed applications tbh

There is no such possibility yet, just make edits to the code on your own, I showed this in the messages above, I hope this will be done in the new version and double rendering will be involved only if there is a specific property specified for this that will activate it.

@makc
Copy link
Contributor

makc commented Aug 4, 2021

it's good enough to show a non-imaginary performance hit due to the double draw calls:

does not seem to be "good eough" @lojjic - it shows things got faster on desktop and how would that be possible?

wait, I misread 0.8 as 0.08 :(

@KiborgMaster
Copy link

@KiborgMaster funny how you had to switch to solid grey floor texture to demonstrate aliasing issue, where your original screenshos had this:
Screen Shot 2021-08-02 at 19 09 04

but I agree that

this should be a separate property of the material.

for the sake of keeping everyone happy

And here is a mini-video of a larger card, imagine here to enable double processing, even when recording a video, I have a slight freeze.

https://www.youtube.com/watch?v=qhV9qeE2Bkc

@makc
Copy link
Contributor

makc commented Aug 5, 2021

@KiborgMaster btw, did you check if this PR affects custom blending code path? maybe you could work around it like that 😅

p.s. just in case,

video of a larger card

=larger map, (card and map are the same word in rus., everyone)

@makc
Copy link
Contributor

makc commented Aug 5, 2021

ok, everyone, I just prayed to the God of Hacks, and - here is your workaround:

https://jsfiddle.net/ny7uco8a/ → 3 calls (2 + 1 opaque)
https://jsfiddle.net/ny7uco8a/1/ → 2 calls (1 + 1 opaque)

power to the people!

🙏

@Alchemist0823
Copy link
Contributor

The I did a profiling on the my batch particle renderer too. It seems the doubling draw call counts is not the reason for performance issue. Because the material.version was set twice for each material per frame. Inside the WebGLRenderer, It calls getProgramCacheKey if the material.version is renewed. getProgramCacheKey takes 30% of my application scripting time. That's why it's so slow.

@titansoftime
Copy link
Contributor

The I did a profiling on the my batch particle renderer too. It seems the doubling draw call counts is not the reason for performance issue. Because the material.version was set twice for each material per frame. Inside the WebGLRenderer, It calls getProgramCacheKey if the material.version is renewed. getProgramCacheKey takes 30% of my application scripting time. That's why it's so slow.

I too am noticing significantly slow getProgramCacheKey in the latest version of three. Similar results of about 30% of js time spent on that function according to chrome profiler. Stuck on earlier build until resolved. There was PR made not too long ago that altered that function aiming to improve it's performance. I assume that made it worse but hard to say without additional testing.

@pailhead
Copy link
Contributor

pailhead commented Feb 1, 2022

One problem here may now be that 13X was out in the wild long enough for people to start projects with it. If someone's project is rendering the torus from the OP reverting this may break it. Instead of forcing the user to do A or B, it may be better to let the user choose between A or B. Webgl has some hints like this eg dont care nicest fastest etc.

I too would like to cast my vote for the previous behavior. Rather than hard coding this functionality, i think it may be possible to achieve it at an app level?

@ingun37
Copy link
Contributor

ingun37 commented May 31, 2022

I recently updated ThreeJS from r121 -> 139 and the FPS's dropped by half. Please consider making this feature optional.🙏

@TimPietrusky
Copy link

TimPietrusky commented Aug 26, 2022

This makes using THREE.DoubleSide impossible on "more than a single torus"-scenes, especially in combination with THREE.Texture.

👍 for opt-in
💯 for adding a THREE.DoubleSide will do a render for both the Front- and the Backside, which might reduce the FPS in complex scenes, especially when using a THREE.Texture.-like hint in the docs so that people are aware and don't have to search for this PR

@mrdoob
Copy link
Owner Author

mrdoob commented Dec 22, 2022

I think we finally figured out a good solution for this: #25165

  1. After r148, THREE.DoubleSide will render in a single pass again.
  2. If someone needs two-pass double sided they have to do material.side = THREE.TwoPassDoubleSide.

If anyone has suggestions for a better name, we're all ears!

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.