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

Improve render order for SVGLoader example #22912

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Conversation

tangobravo
Copy link
Contributor

  • Set all materials as transparent, so all will be rendered as part of the same render list
  • Ordering will fall back to the final object.id ordering in reversePainterSortStable
  • Add an example SVG that is rendered in the correct order after this change

Related issue: #22898

Description

SVGLoader generates multiple meshes that need to be rendered in-order. Before this commit, the transparent attribute of the generated materials was set based on the fill or stroke opacity, which separated materials into the opaque and transparent render lists, and gave the incorrect render ordering.

Additionally as all the meshes are in the z=0 plane (SVG is a 2D format), depthWrite is set false for all of the materials. This is another reason to have all SVG meshes in the transparent render list, as it means they will behave correctly with respect to 3D meshes in the scene that do render into the depth buffer.

- Set all materials as transparent, so all will be rendered as part of the same render list
- Ordering will fall back to the final object.id ordering in reversePainterSortStable
- Add an example SVG that is rendered in the correct order after this change
@mrdoob mrdoob added this to the r136 milestone Nov 29, 2021
@mrdoob mrdoob merged commit a375e5a into mrdoob:dev Nov 29, 2021
@mrdoob
Copy link
Owner

mrdoob commented Nov 29, 2021

Thanks!

@mrdoob mrdoob mentioned this pull request Nov 29, 2021
@@ -193,7 +194,7 @@
const material = new THREE.MeshBasicMaterial( {
color: new THREE.Color().setStyle( strokeColor ),
opacity: path.userData.style.strokeOpacity,
transparent: path.userData.style.strokeOpacity < 1,
transparent: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt users will understand this subtlety. Doing so requires knowledge of the inner-workings of the renderer...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But do you think they'll notice any change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If setting non-transparent objects to transparent is required for three.js to sort and render svg objects correctly, then what we have here is a three.js limitation -- or bug.

The work-around in this example needs to be explained with some comments, no?

Copy link
Contributor Author

@tangobravo tangobravo Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's particularly a three.js limitation - SVG isn't a 3D format and so a best-effort attempt at achieving SVG semantics in a 3D engine involves ensuring the individual meshes are all depthWrite: false (to avoid depth fighting) and rendered in order regardless of their opacity; which in three.js implies setting all of them transparent.

I'd argue that transparent: true is likely the right choice whenever materials are set depthWrite: false as the ability to arbitrarily order draw calls without affecting the output relies on both depthTest and depthWrite being used.

I'd be happy for a comment to be added (perhaps applying to both the depthWrite and transparent lines for the opaque meshes) that either points here or to #22898 if you guys think it helps understanding.

Potentially three.js could also use some better way of ensuring ordering among a particular set of meshes (perhaps via using a single BufferGeometry with groups) but then it's hard to know how that whole group should sort with respect to everything else in the scene.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your explanation in the main post was excellent. I'd be happy with a comment in the code if @mrdoob and @Mugen87 agree.

Copy link
Collaborator

@Mugen87 Mugen87 Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely link to this discussion. A simple addition of // #22912 should be sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple addition of // #22912 should be sufficient.

That would be fine for source code. But for an example, I think a short explanation is warranted,

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to add the comment where transparent: true is set in the example. Of course an additional explanation is fine.

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.

4 participants