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

Fixed a rounding error in the border radius color lerp #203

Merged
merged 3 commits into from
Jul 10, 2021

Conversation

jac8888
Copy link

@jac8888 jac8888 commented Jul 9, 2021

The lerp on the colors in the border radius code sometimes had an error that happened because the result was truncated instead of rounded.

It could be seen when blending two colors that were the same. The points along the arc would have different colors, like this:

DEBUG: color is 255, 224, 102
DEBUG: color is 255, 223, 101
DEBUG: color is 255, 223, 101
DEBUG: color is 254, 223, 101
DEBUG: color is 254, 223, 101
DEBUG: color is 255, 224, 102
DEBUG: color is 255, 224, 102

This is what this behavior would look like:
qwerty

I didn't modify the lerp function itself. I only changed the way it is called in this specific case. There may be other places where a similar fix is needed.

Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for contributing back to the project!

Could you use Math::RoundToInteger instead? Then you can also drop the math.h header.

I think long-term we might want to make an overload in lerp for Colourb, but this is alright for now. I think we also have a color interpolation function in the animation system which properly interpolates in linear color space. That one might be the correct one to use here as well, but it needs to be tested for the same rounding issue.

@jac8888
Copy link
Author

jac8888 commented Jul 9, 2021

Thank you for maintaining the project!

Absolutely, for now I've made only the change to the rounding function.

I found the code you were talking about in the ElementAnimation. I hadn't even considered the color space. I can do some more testing with the rounding on it next week and see if it will be good to use here as well.

@mikke89
Copy link
Owner

mikke89 commented Jul 10, 2021

Thank you, sounds good! I'll merge this in the meantime.

@mikke89 mikke89 merged commit 747d73c into mikke89:master Jul 10, 2021
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.

2 participants