-
-
Notifications
You must be signed in to change notification settings - Fork 36.2k
Line2NodeMaterial: Align wide lines with WebGLRenderer version.
#32234
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
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
|
|
||
| // adjust for clip-space to screen-space conversion // maybe resolution should be based on viewport ... | ||
| offset.assign( offset.div( viewport.w ) ); | ||
| offset.assign( offset.div( viewport.w.div( screenDPR ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bug occurred because the definitions are not consistent.
renderer.setViewport() is in logical pixel units. Can you improve the nomenclature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a difference with what is defined via the renderer API and what TSL objects represent in the shader. That has been mentioned here #29347 (comment). I don't think you can come up with a solution that satisfies every use case. Keeping the TSL objects in physical pixel units seems correct to me. And I would leave the renderer API as it is, especially to avoid any kind of breakage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the TSL objects in physical pixel units seems correct to me.
Yes.
And I would leave the renderer API as it is,
Yes.
So the only possible changes are in the margins: revist the docs and inline docs and ensure the units are clearly stated.
Also the internal nomenclature could be reviewed.
_viewport // logical (css) pixel units
_currentViewport // physical pixel units
state.viewport // physical pixel units
Fixed #32229.
Description
As suggested in #32229 (comment), the PR makes sure the wide line implementation for
WebGPURendereruses CSS pixel units to ensure consistent line widths across devices. For this, the DPR must be factored out of the viewport.With this PR, the wide line demos now render with equal width for
WebGPURendererandWebGLRenderer.