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

GFLBMassNode to extend MassNode? #150

Closed
zepumph opened this issue Jun 17, 2019 · 3 comments
Closed

GFLBMassNode to extend MassNode? #150

zepumph opened this issue Jun 17, 2019 · 3 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jun 17, 2019

Discovered as part of #134. This will save code (a real commodity these days) and complexity, especially duplication in updateGradient.

@zepumph
Copy link
Member Author

zepumph commented Jun 18, 2019

@jessegreenberg @twant and I feel pretty good about this. Could you please review for regression, as well as two new points:

  • Does the way of using options to support inheritance here make sense?
  • Notice that the updateGradient has a conditional about if constantRadius is checked or not. That was not in GFLB. Is it alright that it is now in GFLB? I couldn't see any changes, but you are the responsible dev.

@zepumph
Copy link
Member Author

zepumph commented Jun 18, 2019

Notice that the updateGradient has a conditional about if constantRadius is checked or not. That was not in GFLB. Is it alright that it is now in GFLB? I couldn't see any changes, but you are the responsible dev.

I changed my mind. I can tell a difference when the stroke is present. I think it looks good in GFLB. @jessegreenberg can you think of a reason that it was purposefully omitted?

@jessegreenberg
Copy link
Contributor

Code looks really nice to me, thanks! I searched through commit history and repo issues in here, GFL, and ISLC and couldn't find anything about lack of stroke. I assume that this visual change is totally great. I actually think it looks better. Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants