-
-
Notifications
You must be signed in to change notification settings - Fork 35.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
LUT: Fix color definition and sampling. #23589
Conversation
No, sorry. Maybe I wasn't clear, but rounding is the right thing to do. Here's an example: Let's add a keyword With this PR's Lut: >>> const l = new Lut("blacktowhite", 4);
// this returns lut[1] (0.25), it should return lut[2] (0.5)
// since 0.48 is closer to 0.5 than 0.25
>>> l.getColor(0.48)
Object { r: 0.25, g: 0.25, b: 0.25 }
// behavior for alpha near 1
>>> l.getColor(0.999)
Object { r: 0.75, g: 0.75, b: 0.75 } // bad
>>> l.getColor(1)
Object { r: 1, g: 1, b: 1 } // good! With dev's Lut: // this is correct
>>> l.getColor(0.48)
Object { r: 0.5, g: 0.5, b: 0.5 }
// the only "problem" is for alpha near 1 the color at
// the very end of the ramp doesn't get returned
// (essentially the lut[n] bucket is missing/unused)
>>> l.getColor(0.999)
Object { r: 0.75, g: 0.75, b: 0.75 }
>>> l.getColor(1)
Object { r: 0.75, g: 0.75, b: 0.75 }
// contrast with alpha near 0, where the color at
// the beginning DOES get returned
>>> l.getColor(0.001)
Object { r: 0, g: 0, b: 0 } |
Okay, I've updated the PR. |
Cool, getColor looks right to me. For setColorMap: The loop doesn't always make n+1 elements in lut. For n=11 for example, i ends up being > 1 at the end of the loop, so there is no lut[11] and getColor(1) returns undefined. IMO it would be easier to do something like this // sample at 0
this.lut.push(this.map[0][1])
// sample at 1/n, ..., (n-1)/n
let j = 0;
for (i = 1; i !== n; i++) { // use integer loop counter
const alpha = i * step;
// advance j until alpha is in the right range
...
const color = lerp(...)
// every iteration unconditionally pushes one element
this.lut.push(color)
}
// sample at 1
this.lut.push(this.map[this.map.length - 1][1]) |
Slightly changed your code listing but the result should be okay now. |
Okay. As far as I can tell, it's right now. |
Thanks! |
Sorry guys, I could not have a look at this until now. It really looks good now! Thank you so much @Mugen87!! |
Fixed #23549.
Description
Fixes
getColor()
andsetColorMap()
. ImprovesupdateCanvas()
.@dcriado @scurest Does this look better to you?