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

fixes the semitransparent stroke disabled #5985

Merged
merged 4 commits into from Jan 31, 2023

Conversation

Prateek462003
Copy link
Contributor

Resolves #5951

Changes:
fixed the stroke method . previously it was using the default opaque strokes even if changed by the user

PR Checklist

@inaridarkfox4231
Copy link
Contributor

I don't understand why the warning is needed when it should just allow transparency changes.

@Prateek462003
Copy link
Contributor Author

@inaridarkfox4231 I thought it would be better if the user would know that little detail . I will push the changes right away.

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Just some small cleanup, otherwise this is good to go!

We might want to consider making a follow-up issue to address the behaviour strokes overlapping with each other at corners though. Here's a rectangle:
image

...but even without that solved, this should still enable people to draw single lines with transparency!

console.warn(
'Default opaque stroke is used if not the optional fourth argument is not specified in stroke method');
}
// arguments[3]=255;
Copy link
Contributor

Choose a reason for hiding this comment

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

We aren't using this line any more right? We can take this out too for readability

arguments[3] = 255;
// @todo allow transparency in stroking currently doesn't have
// any impact and causes problems with specularMaterial
if(arguments[3]===undefined){
Copy link
Contributor

Choose a reason for hiding this comment

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

Super tiny formatting change, but mind adding spacing around the === operator to better match the formatting elsewhere in the codebase?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I think we can also take out that @todo comment too. This doesn't seem to affect specularMaterial any more (aside from the stroke joint blending issues I mentioned earlier, which we can address separately.)

@Prateek462003
Copy link
Contributor Author

@davepagurek please review this

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Thanks @Prateek462003, looks good!

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.

Semitransparent strokes disabled in WebGL mode
3 participants