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

LineMaterial: Automatically adjust the USE_DASH define when setting "dashed" #21797

Merged
merged 2 commits into from
May 8, 2021

Conversation

gkjohnson
Copy link
Collaborator

Related issue: --

Description

Adjust the LineMaterial.dashed field behavior to automatically change the material USE_DASH define and set needsUpdate to true. I found it confusing when using this that adjusting dashed seemed to do nothing (and from looking at the code it wasn't being used at all).

Live demo here:

https://raw.githack.com/gkjohnson/three.js/improve-dash-field/examples/webgl_lines_fat.html

cc @WestLangley

@WestLangley
Copy link
Collaborator

As @WestLangley said,

dashed is implemented as a defines -- not as a uniform. this could be changed.

So, I agree with what you are doing, but what is your reason for not implementing it as a uniform -- like the other properties?

@mrdoob mrdoob added this to the r129 milestone May 7, 2021
@gkjohnson
Copy link
Collaborator Author

My goal was to simplify the material use not necessarily change the underlying implementation. I can move that comment to the material if you'd like.

Regarding whether I think dashed should be set as a define or a uniform I tend to lean towards using defines when I don't think a field will change often and removing it would add a branch. Because the branch would be based on a uniform value, though, it's possible that it wouldn't cause any performance issues because every fragment would run the same code path but I'm not confident in my general understanding of GPU behavior here. However I know a define will at best be faster among the two options (and at worst equal).

If you feel strongly one way or the other I'm happy to change it, though.

@WestLangley
Copy link
Collaborator

If you feel strongly one way or the other I'm happy to change it, though

I don't. Thank you for the PR!


}

if ( value ) {
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this should be more robust?

if ( value === true ) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done! Change the alpha to coverage setter, too.

@mrdoob mrdoob merged commit 9b0ed26 into mrdoob:dev May 8, 2021
@mrdoob
Copy link
Owner

mrdoob commented May 8, 2021

Thanks!

@gkjohnson gkjohnson deleted the improve-dash-field branch May 8, 2021 07:45
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.

3 participants