Fix color validation by avoiding null vectors between identical colors & running all checks concurrently #17272
Conversation
|
Woops, this actually only fixes the first part of issue #16837, where the range adjustment didn't get done. The second part, where the two checks aren't run concurrently, isn't fixed yet. |
|
Now I've made a fix for the whole issue #16837, which ensures that all checks are made at the same time. |
robpvn
changed the title
|
Sorry for looking at this only so late. The code changes look good to me on a first glance. I'd agree that picking a random new colour is worse than just adjusting the colour slightly, so the behaviour in this PR should be fine. One other thing I noticed: You should remove the |
robpvn
force-pushed
the
fixColourPicker
branch
from
7d5b737
to
843439a
Compare
|
@abcdefg30 Don't worry about responding late, it turns out I'm slow to respond as well! :D I deleted the unused variable and rebased the commits to the newest bleed. |
|
I opened #17582 so we don't forget about it once this PR is merged. |
|
LGTM otherwise, and it makes sense to get this in for the playtest so adding to the milestone. Given the time constraints on the playtest release, I'll apply the fixups and merge this next weekend if not fixed sooner. |
The failsafe in ColorValidator aborts after 255 iterations of adjusting the color and picks a random color. This message makes it clearer to the user. Results in two messages being displayed, first one about adjusting and the about a random color pick.
If the picked color and a forbidden color are identical (like if they both picked the same palette color and in the special case when a picked color is outside of the allowed range and the method returns the picked color as the forbidden color), the vector between them is zero and the maths for adjusting the color fails by hitting the iteration limit. This changes the zero vector to the smallest possible vector in order to avoid the issue. This can result in some seriously close adjustments in the case of picking identical palette colors, which might be undesirable compared to picking a new palette color.
pchote
force-pushed
the
fixColourPicker
branch
2 times, most recently
from
7a938a9
to
21b6504
Compare
This ensures that color picks that have multiple issues will have them all checked at the same time, including ensuring that the fix for one issue doesn't cause another issue. Handling of the onError action has been changed from being called at once to collecting the potential errors in a HashSet to deduplicate them and then calling onError after a valid color has been found. (Otherwise you would in the worst case get 256 error messages logged!)
pchote
force-pushed
the
fixColourPicker
branch
from
21b6504
to
f3cc1ae
Compare
|
Sorry about not responding promptly, I was laid up with the flu last week. Thanks for the merge! |
This is a fix for issue #16837 , which also has some (possibly) unintended consequences worth discussing.
If the picked color and a forbidden color are identical (like if they both picked the same palette color and in the special case when a picked color is outside of the allowed range and the method
returns the picked color as the forbidden color), the vector between them is zero and the maths for adjusting the color fails by hitting the iteration limit. This changes the zero vector to the smallest possible vector in order to avoid the issue.
I also added an extra message if you hit the iteration failsafe that picks a random colour.
However, this can result in some seriously close adjustments in the case of picking identical palette colors, which might be undesirable compared to picking a new palette color. (See picture of adjusted identical palette picks below.) While technically the current behaviour of picking a random new palette color is a bug, I have a feeling many people might see it as a feature. Comments?