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

Examples: Remove need for manually assigning Line2 material resolution #28343

Merged
merged 11 commits into from
May 14, 2024

Conversation

gkjohnson
Copy link
Collaborator

Related issue: #28335

Description

Remove the need to manually assign the resolution uniform for rendering Line2. If rendering to multiple resolutions of canvas or render targets then the resolution still needs to be set before performing raycasting.

@gkjohnson gkjohnson added this to the r165 milestone May 12, 2024
@WestLangley
Copy link
Collaborator

WestLangley commented May 12, 2024

Nice. This is working for me.

I think there are a few more lines that can be edited. Grep for "resolution" in the relevant examples.

Perhaps unrelated, there is also a webgpu example. It renders lines with a slightly different width for some reason. /ping @sunag

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented May 12, 2024

I think there are a few more lines that can be edited. Grep for "resolution" in the relevant examples.

There was one straggler - thanks!

Perhaps unrelated, there is also a webgpu example. It renders lines with a slightly different width for some reason.

I'm not familiar enough with all the WebGPU stuff yet to make changes here

@WestLangley
Copy link
Collaborator

There are additional comments to remove:

//resolution:  // to be set by renderer, eventually

@gkjohnson
Copy link
Collaborator Author

There are additional comments to remove:

Thanks, missed those

@aardgoose
Copy link
Contributor

I think there are a few more lines that can be edited. Grep for "resolution" in the relevant examples.

There was one straggler - thanks!

Perhaps unrelated, there is also a webgpu example. It renders lines with a slightly different width for some reason.

I'm not familiar enough with all the WebGPU stuff yet to make changes here

The viewport uniform in the nodes material is automatically updated so there is no need for changes there. Not sure where the width variation comes from though.

@WestLangley
Copy link
Collaborator

@gkjohnson The line widths are different compared to those in the previous release. At least on my machine.

I expect you are off by a factor of pixelRatio. You have to differentiate between CSS pixel units and device pixel units when sizing the viewport.

@gkjohnson
Copy link
Collaborator Author

I expect you are off by a factor of pixelRatio. You have to differentiate between CSS pixel units and device pixel units when sizing the viewport.

Oh interesting - I guess WebGLRenderer.getSize does not include the pixelRatio and WebGLRenderer.getViewport does. I've fixed it by multiplying the pixel ratio out of the viewport and it looks the same on my mac, now.

@WestLangley
Copy link
Collaborator

I guess WebGLRenderer.getSize does not include the pixelRatio and WebGLRenderer.getViewport does.

No, that is not true. getViewport() returns results in CSS logical pixel units.


if ( this.material.uniforms.resolution ) {

renderer.getCurrentViewport( _viewport ).multiplyScalar( 1 / renderer.getPixelRatio() );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be:

renderer.getViewport( _viewport );

@aardgoose
Copy link
Contributor

@WestLangley

Is the line width discrepancy you see in the webgpu version the width in the inset box when in css pixel width mode?

If so I have found the issue that creates that difference.

@WestLangley
Copy link
Collaborator

@aardgoose

Is the line width discrepancy you see in the webgpu version the width in the inset box

yes

@gkjohnson
Copy link
Collaborator Author

No, that is not true. getViewport() returns results in CSS logical pixel units.

Gotcha thanks - just fixed. The naming of getViewport vs getCurrentViewport is not super clear 😅

@gkjohnson
Copy link
Collaborator Author

The naming of getViewport vs getCurrentViewport is not super clear 😅

Oh man even the documentation between the two is identical:

Returns the current viewport.

I'm wondering if this should just be changed to a flag to be passed into getViewport to indicate css or real pixels.

Mugen87
Mugen87 previously approved these changes May 14, 2024
@@ -7,7 +7,7 @@
* dashSize: <float>,
* dashOffset: <float>,
* gapSize: <float>,
* resolution: <Vector2>, // to be set by renderer
* resolution: <Vector2>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line should be removed.

@Mugen87 How do you feel about removing this entire comment block? Apparently, we no longer use this coding pattern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll fix it. It's time to move on.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 14, 2024

Um, does webgpu_lines_fat need an update? The E2E test seems to fail.

@WestLangley
Copy link
Collaborator

@aardgoose
@sunag

With this PR, webgpu_lines_fat.html fails in .onBeforeRender() due to this reference:

this.material.uniforms.resolution

This fixes it:

this.material.uniforms?.resolution

Is there a "proper" way of handing this?

@sunag
Copy link
Collaborator

sunag commented May 14, 2024

I think the best solution at this is your code this.material.uniforms?.resolution or this.material.uniforms !== undefined && ..., I don't remember if we had a reason not to use optional chaining? /@Mugen87.

The Nodes update the values individually so we don't need a uniform object for each material. I think that soon we would also have to review the implementation of the default material LineMaterial to avoid GLSL code within a possible compilation of the code too.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 14, 2024

I don't remember if we had a reason not to use optional chaining?

We have reverted optional chaining because we want to stick to ECMA 2018 syntax, see #25172.

@Mugen87 Mugen87 dismissed their stale review May 14, 2024 21:10

WebGPU example breaks.

@Mugen87 Mugen87 merged commit 148f9b1 into mrdoob:dev May 14, 2024
11 checks passed
@gkjohnson gkjohnson deleted the line2-resolution branch May 14, 2024 23:44
@MiguelG97
Copy link

MiguelG97 commented Jul 13, 2024

FYI:
hey guys in this thread
https://stackoverflow.com/questions/78742570/three-js-fat-lines-line-width-is-exaggerated/78742578#78742578
we still need to pass the resolution parameter other way it's behaving strange.

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