-
Notifications
You must be signed in to change notification settings - Fork 206
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
ColorChooser : Maintain hue and saturation at zero #5909
Conversation
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.
Thanks Eric - very nice to have this fixed. Couple of comments inline - the latter of which is definitely worth addressing I think. And we also need to target the PR to 1.4_maintenance1
.
Cheers...
John
5498333
to
adc7559
Compare
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.
Thanks Eric! And sorry, one last round of comments from me. The "not public" one is definitely important - see what you think about the others...
Cheers...
John
I pushed up a fix to the previous fix (squashed into the fixup in f2fd617) and all seems to be working well in practice. Ready for a new look! |
f2fd617
to
5b1070c
Compare
Thanks Eric! I've squashed in the fixups and merged. |
We want all our ducks in a row before being exposed to the arbitrary code execution represented by connected slots. I haven't observed any problems from the old code in practice, but noticed the pre-exising bug while reviewing GafferHQ#5909 and thought it probably worth addressing just in case.
We want all our ducks in a row before being exposed to the arbitrary code execution represented by connected slots. I haven't observed any problems from the old code in practice, but noticed the pre-exising bug while reviewing GafferHQ#5909 and thought it probably worth addressing while we're looking at this part of the codebase.
This changes the behavior of the color chooser somewhat, so that setting the saturation to zero does not cause the hue to be reset to zero, and setting the value to zero does not cause the hue and saturation to be reset to zero.
I'm not super-excited about storing and syncing the HSV color to the RGB color, but I think it's better than the alternatives. We need some way of knowing what the HS value should be if we're deciding not to set it, and that's now coming from the synchronized
self.__colorHsv
variable.I also looked at converting the whole
ColorChooser
to use HSV values internally and not store RGB. That worked also, but as we talked about, it also has the side effect thatColorChooser.setColor( x ).getColor() != x
because of floating point round off, which does not seem like the right way to go.Checklist