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

WebGPU: Introduce Fat Points #26930

Merged
merged 11 commits into from
Oct 14, 2023
Merged

WebGPU: Introduce Fat Points #26930

merged 11 commits into from
Oct 14, 2023

Conversation

WestLangley
Copy link
Collaborator

@WestLangley WestLangley commented Oct 9, 2023

As suggested in #25697 (comment).

Based on #11349. A "fat point" sized in pixel units is just a "fat line" of zero length.

Screenshot 2023-10-09 at 4 12 50 PM

Note, these "fat" points are sized in pixel units. This is different than the THREE.Points implementation.

//

So, what features do we want to support?

How should this be integrated into Core?

In particular, what should be the API?

@WestLangley WestLangley marked this pull request as draft October 9, 2023 20:33
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 10, 2023

TBH, I'm not really a fan of the Fat* syntax. I would call the class InstancedSprite (and InstancedSpriteGeometry and InstancedSpriteMaterial).

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 10, 2023

Sidenote: At some point we should maybe rename examples like webgl_lines_fat to webgl_lines_wide or something similar.

@Mugen87 Mugen87 added this to the r??? milestone Oct 10, 2023
@WestLangley
Copy link
Collaborator Author

WestLangley commented Oct 10, 2023

I would call the class InstancedSprite

But these are not sprites. They are points, sized in pixels. When re-implemented in TSL, they would replace the single-pixel points rendered by WebGPURenderer.

Instanced sprites have a map, are sized in world units, can be size-attenuated, and can be rotated.

I am inclined to think we are talking about two different things.

I'm not really a fan of the Fat* syntax.

That's fine. Different nomenclature can be used.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 10, 2023

We can also call them InstancedPoints. I think we originally did want to use the "points" term because they are not based on the points primitive.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 11, 2023

Regarding #25697 (comment):

With this implementation, is the point size now consistent when changing the renderer's pixel ratio?

@WestLangley
Copy link
Collaborator Author

With this implementation, is the point size now consistent when changing the renderer's pixel ratio?

The user should not have to worry about the device pixels.

pointWidth here is in CSS Pixels, and the point size is invariant to window.devicePixelRatio.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 11, 2023

pointWidth here is in CSS Pixels, and the point size is invariant to window.devicePixelRatio.

That is good since it solves one issue with previous point primitive rendering.

@WestLangley
Copy link
Collaborator Author

WebGPURenderer is now supported. Many thanks to @aardgoose for #26704.

The "Fat Points" nomenclature can be changed.

import { OrbitControls } from 'three/addons/controls/OrbitControls.js';

import { FatPoints } from 'three/addons/points/FatPoints.js';
//import { FatPointsNodeMaterial } from 'three/addons/points/FatPointsNodeMaterial.js'; // why not this, instead?
Copy link
Contributor

Choose a reason for hiding this comment

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

You can actually use just FatPointsMaterial and it would be auto-converted to FatPointsNodeMaterial by WebGPURenderer based on its name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here I just meant that you can import just the FatPointsMaterial instead of FatPointsNodeMaterial and it should work exactly the same.
For background I believe setting scene.background = THREE.Color( 0x222222 ) should also work just fine as scene.backgroundNode = color( 0x222222 ). So basically using the same code as for WebGL should work 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we no longer need FatPointsMaterial. My intention is to remove it. (I just wanted to get its shader to work correctly before attempting the Node version.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can actually use just FatPointsMaterial and it would be auto-converted to FatPointsNodeMaterial

It does render, but the vertex colors are not honored.

Assuming that will be fixed, would it be preferable to retain FatPointsMaterial (in glsl) and remove FatPointsNodeMaterial?

ping @sunag @LeviPesin

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the main scope is WebGPURenderer, I vote yes.

It's great to see NodeMaterial saved lines of code in relation to the same material created in ShaderMaterial, with the advantages that each "uniform" is a channel for a procedural node.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For background I believe setting scene.background = THREE.Color( 0x222222 ) should also work just fine as scene.backgroundNode = color( 0x222222 ).

For WebGPURenderer (both backends), it appears

scene.background = new THREE.Color( 0xff0000 ); // does not honor viewport/scissor

scene.backgroundNode = color( 0xff0000 ); // honors viewport/scissor

Copy link
Collaborator

@sunag sunag Oct 13, 2023

Choose a reason for hiding this comment

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

scene.background = new THREE.Color( 0xff0000 ); // does not honor viewport/scissor

Could this be happening there because scene.backgroundNode overrides the scene.background settings?
I tested it in webgpu_lines_fat replacing all scene.backgroundNode to null and work. (or just remove all scene.backgroundNode assigns)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sunag For me, the background color of the inset view extends to the entire screen when scene.background is used in WebGPURenderer.

Copy link
Contributor

Choose a reason for hiding this comment

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

scene.background = new THREE.Color( 0xff0000 ); // does not honor viewport/scissor

Could this be happening there because scene.backgroundNode overrides the scene.background settings? I tested it in webgpu_lines_fat replacing all scene.backgroundNode to null and work. (or just remove all scene.backgroundNode assigns)

Setting the background color using the clear color doesn't work in the same way as WebGLRenderer. The scissor area does not constrain the region cleared.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

/ping @sunag @LeviPesin See comment by @aardgoose .

@WestLangley WestLangley changed the title Fat Points: Proof of Concept WebGPU: Introduce Fat Points Oct 14, 2023
@WestLangley WestLangley marked this pull request as ready for review October 14, 2023 03:15
@WestLangley WestLangley modified the milestones: r???, r158 Oct 14, 2023
@sunag sunag merged commit c44c4ec into mrdoob:dev Oct 14, 2023
18 checks passed
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 14, 2023

Please remove the Fat* syntax though. I vote for InstancedPoints*.

@WestLangley WestLangley deleted the dev-fat_points branch October 14, 2023 16:44
@WestLangley
Copy link
Collaborator Author

@sunag Let's also consider calling them Points, and making this transparent to the user. I think we should have a discussion about this.

@mrdoob
Copy link
Owner

mrdoob commented Oct 26, 2023

@sunag Let's also consider calling them Points, and making this transparent to the user. I think we should have a discussion about this.

I vote for this too 👍

We could, instead, introduce BasicPoints to allow people use the GL_POINTS / point-list code path?

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.

6 participants