fix material colors when less than 5 #2236
Conversation
IntegratedQuantum
left a comment
IntegratedQuantum
left a comment
There was a problem hiding this comment.
Please also adjust the value range to actually use the full spectrum of values, otherwise if there are 10 values, it will only ever use the first 5 of them.
|
yea idk man the issue said for colors below 5 it said nothing about more and i absolutely do not understand the math is doing to do what u want (also i gtg) |
|
Well, clamping would also add a heavy bias for less than 5. |
IntegratedQuantum
moved this from Easy to Review
to WIP/not ready for review
in PRs to review
|
ok after many tries, blind guesses and almost crashing out i got to this point but my current issue now is the fact the tools look bad at 5 colors but nice at 10 |
|
(ignore the gold zon im gonna clean it up if this is good enough) |
|
You should be able to make something that looks the same as before for 5 colors. |
|
ill try editing it a bit then since i have time |
|
oh i think i was just casting not enough* light this seems to be the same as before |
|
(forgot to remove the floor from mid) |
(otherwise it goofs up the gradient when it has a decimal value)
|
nvm im stupid i forgot i needed that to even out the center colors |
|
ok should be ready for merge |
| const mid = @as(f32, @floatFromInt(material.colorPalette.len - 1))/2.0; | ||
| const lightScale = mid/2; | ||
| const lightVal = (lightTL*2.0 + lightTR)/6.0*lightScale; | ||
| var light = @as(i32, @intFromFloat(@round(@floor(mid) - lightVal))); |
There was a problem hiding this comment.
Why is it flooring the mid? To me it seems like this would add a bias when the number of colors is even.
There was a problem hiding this comment.
without the floor the shading is just messed up
| light = @max(@min(light, 4), 0); | ||
| const mid = @as(f32, @floatFromInt(material.colorPalette.len - 1))/2.0; | ||
| const lightScale = mid/2; | ||
| const lightVal = (lightTL*2.0 + lightTR)/6.0*lightScale; |
There was a problem hiding this comment.
There are two magic values here (6.0 should be 3.0, and the /2 in the line above), if you can't find a good name for them, please at least combine them into a single /4.
There was a problem hiding this comment.
ok sherlock how do u want me to combine them then
/3.0 x mid breaks the math in general
/(3.0 x mid) breaks when theres 10 gradients
complaining about magic numbers here is just angers me, because those magic numbers are the exact leftovers u left me to work with and figure out what are they there for
There was a problem hiding this comment.
Just because the original code was bad, doesn't give you permission to make things worse.
What I'd suggest is lightScale = mid/4.0 and lightVal = (...)/3.0*... that way we only have one purely magic number instead of two.
Ideally of course you'd get rid of both magic numbers, but I think that would require a larger rewrite of the math, which doesn't seem like a good idea right now.
There was a problem hiding this comment.
what? this doesnt get rid of any magic numbers unless ur telling me to get rid of the /2 at mid (which is not a magic number cuz its pretty obvious by name im getting the mid point here) but that completly breaks the math
There was a problem hiding this comment.
The /3.0 is less magic because it's clearly part of an interpolation between two numbers.
|
is this abandoned? |
3 participants
fixes #2235