Skip to content
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 : TMI Sliders #5962

Merged
merged 3 commits into from
Jul 19, 2024
Merged

Conversation

ericmehl
Copy link
Collaborator

This adds three new color sliders to the ColorChooser : temperature, magenta and intensity. The conversion between TMI and RGB is based on the formulas from https://www.comp-fu.com/2012/05/tmi-color-temperature-correction/ and modified to account for the different between grading an existing color and generating one.

This is the first in a few PRs adding features to ColorChooser. Our intention is to not expose the changes until a final PR to incorporate all the changes, because the layout in particular is likely to evolve as additional features are added. The last commit is meant as a testing commit to test this PR. We will drop it before actually merging.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Eric - seems to work well. I don't have much experience of using TMI sliders so I was surprised at how easy it is to end up with negative RGB while modifying TMI, but I see that a certain other package doesn't protect against that, so maybe folks will be savvy to that already?

python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUITest/ColorChooserTest.py Outdated Show resolved Hide resolved
python/GafferUITest/ColorChooserTest.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
@ericmehl
Copy link
Collaborator Author

Thanks John! I addressed the comments, and stuck my head in the sand for a bit longer on the column vs. grid-only layout question, if that's ok.

It is odd how some components go outside the 0-1 range fairly easily. I'm not sure what a better behavior would be, the other DCCs I've tested allow it, as you noted, so I guess artists just need to be aware of it.

Maybe we could consider some kind of warning indicator when a channel goes outside the 0-1 range?

Ready for a new look!

@johnhaddon johnhaddon changed the base branch from main to 1.4_maintenance July 18, 2024 09:20
@johnhaddon
Copy link
Member

I addressed the comments

Thanks! I've squashed the fixups in preparation for a merge, and also changed the target to 1.4_maintenance - I think that was the plan?

and stuck my head in the sand for a bit longer on the column vs. grid-only layout question, if that's ok.

Totally fine.

Maybe we could consider some kind of warning indicator when a channel goes outside the 0-1 range?

I've pushed one further commit which explores this idea using the existing "out of range triangle" that the sliders have. It means loosening the restrictions on the slider interaction slightly, but I think it's a net win, and I think this subtle indicator is probably enough compared to anything more drastic, especially if folks are used to the gotchas elsewhere. See what you think...

@ericmehl
Copy link
Collaborator Author

I think this subtle indicator is probably enough compared to anything more drastic

That looks good to me, nice to keep it consistent with how it's been shown already.

I dropped the temporary commit in anticipation of merging. ( I have it locally for later or if you want me to add it back in for more testing). And yes, merging this to 1.4 was my intention, thanks for catching that.

@johnhaddon
Copy link
Member

Looks like the previous commits have a dependency on the temp commit.

ericmehl and others added 3 commits July 18, 2024 10:56
This was broken slightly in GafferHQ#5902 when we introduced channel name
labels. The text width varies somewhat, causing the numeric widget and
sliders to be imperfectly aligned.
We don't really want to allow these, but they can occur naturally in one
component while editing values in another. By accommodating that within the
slider `hardMin`, we get a nice "out of range" triangle to alert us when we've
gone negative, whereas before the slider clamped to zero and gave us a misleading
value. For consistency, we now also allow negative values to be entered directly
in the numeric widgets - if you can get into that state using a slider or numeric
widget for another compoment, why preclude it in the field itself?
@ericmehl
Copy link
Collaborator Author

It was attempting to set the slider value for a slider that didn't exist. Fixed and squashed into f8f0945. Should be good to go now.

@johnhaddon johnhaddon merged commit 6a95643 into GafferHQ:1.4_maintenance Jul 19, 2024
5 checks passed
@ericmehl ericmehl mentioned this pull request Aug 16, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants