-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Fix geometry entity with dynamic color #6245
Conversation
hpinkos
commented
Feb 21, 2018
- Fixes dynamic color when the result parameter is not used
- Fixes dynamic depth fail color
@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
if (!Color.equals(attributes._lastColor, colorScratch)) { | ||
attributes._lastColor = Color.clone(colorScratch, attributes._lastColor); | ||
attributes.color = ColorGeometryInstanceAttribute.toValue(colorScratch, attributes.color); | ||
var resultColor = colorProperty.getValue(time, colorScratch); |
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 is a perfect example for why I think we should always prefer
var result = someFunction(resultParameter)
// do more things with result
instead of
someFunction(resultParameter);
//do more things with resultParameter
I think it makes the code easier to follow for someone who is not used to how we use result parameters, and it avoids mistakes like this where the result parameter might not actually be used.
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 agree with you. I think this is in our coding guide, can you check and open a PR if not? Thanks.
Does this fix #5371? |
Nope |
@ggetz can you review? |
I have a feeling there are quite a few random bugs that this will fix, whether we know it now or not. |