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

Color Chooser Constrain Drag #6128

Open
wants to merge 2 commits into
base: 1.5_maintenance
Choose a base branch
from

Conversation

ericmehl
Copy link
Collaborator

This lets users constrain dragging in the color field to a single axis (or circle if the wheel is the current widget) by holding the Control key.

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.

@murraystevenson
Copy link
Contributor

Thanks Eric!

I wonder if we should consider constraining based on the current value when Ctrl clicking on the colour field? This would be for the "I want to keep my current saturation, but swing the hue to over there" sort of interactions, which I imagine would be quite common? Currently I find it quite hard to use the constrained drag to adjust one component of the original colour without also making an adjustment to the other component, such as only adjusting hue without also making a minor adjustment to saturation. It feels like it would be much more straightforward if I could hold Ctrl and click in another region of the colour field to set the hue knowing that the saturation hasn't changed, then drag to further refine the hue at the original saturation.

I think for this to make sense we'd also need to change the constraint overlay to be visible when Ctrl is held and the left mouse is pressed, which would also be a small increase in discoverability over the current behaviour which requires you to hold Ctrl and drag to make the overlay visible.

@ericmehl
Copy link
Collaborator Author

ericmehl commented Nov 4, 2024

That does sound like a good interaction, I'll make that change and see how it works. I tended to start from a click and drag the indicator rather than click on the opposite side, but was probably getting some small unintended changes like you were.

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

Summarizing my inline responses, 4df9e88, 66cdf23 and 068e77d implement an overhauled constrained drag strategy that I think is better than before and handles the corner cases well.

Ready for a new look and testing.

Copy link
Contributor

@murraystevenson murraystevenson 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! The interaction is feeling much better with these latest changes. I have one question inline about the snap to center behaviour but otherwise we're looking pretty close.

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

In 172bb57 I removed the near-center snapping behavior, and made what is hopefully the final tweak to dragging. Previously, I was using the distance from center as the saturation / value constraint to the radius line. That meant that if you were dragging from the opposite side of the circle from the start point, far from the center, you'd get a sudden snap to close to the end of the constraint line as soon as you crossed back to the same side of the circle as the start point.

172bb57 constrains the point to the closest point on the radius line, making for a smooth transition when crossing sides at any position.

Copy link
Contributor

@murraystevenson murraystevenson left a comment

Choose a reason for hiding this comment

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

Thanks! The removal of the snapping works great for me. A pair of final nitpicks inline, but they can be squashed in while preparing to merge.

python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
python/GafferUI/ColorChooser.py Outdated Show resolved Hide resolved
def __init__( self, color = imath.Color3f( 1.0 ), staticComponent = "v", dynamicBackground = True, **kw ) :
__DragConstraints = enum.Flag( "__DragConstraints", [ "X", "Y" ] )

def __init__( self, color = imath.Color3f( 1.0 ), staticComponent = "v", dynamicBackground = False, **kw ) :
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhere along the way we've ended up with dynamicBackground = False here.

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