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 crash in gradient sampling #3828
Conversation
CC: @Dherse
@@ -1249,7 +1249,11 @@ fn sample_stops(stops: &[(Color, Ratio)], mixing_space: ColorSpace, t: f64) -> C | |||
|
|||
let (col_0, pos_0) = stops[low - 1]; | |||
let (col_1, pos_1) = stops[low]; | |||
let t = (t - pos_0.get()) / (pos_1.get() - pos_0.get()); | |||
let mut delta = pos_1.get() - pos_0.get(); | |||
if delta == 0.0 { |
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.
can we get into trouble if delta
isn't equal to 0.0
, but very close to it?
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.
The current way this is dealt with seems somewhat arbitrary to me. Wouldn't it make more sense to simply make t either 0.0, 0.5 or 1.0 in the event that two stops are the same?
Maybe I misunderstand what's going on though.
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. As-is it behaves as if the two stops were actually very far apart (the full 100% = 1.0
) and which of the stops "wins" depends on where the stops are along the gradient.
https://discord.com/channels/1054443721975922748/1088371867913572452/1224684053870153848 Some minor discussion on discord |
if delta == 0.0 { | ||
delta = 1.0; | ||
} | ||
let t = (t - pos_0.get()) / delta; |
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.
We can also deduplicate stops beforehand, but I think we should in any case clamp t
to 0.0..=1.0
here. You never know what's gonna happen with floats.
@frozolotl still on your radar? |
Closing due to inactivity. Feel free to reopen if you still want to work on this! |
Closes #3826.
CC: @Dherse