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

Constrained rotation #1226

Merged
merged 10 commits into from Jul 2, 2019

Conversation

scribblemaniac
Copy link
Member

@scribblemaniac scribblemaniac commented Jun 18, 2019

This PR enables constrained rotation for rotating selections. So when rotating selections, if you hold the shift key, the rotation will snap to the closest angle that's a multiple of n. n is 15 by default, but can be modified in the preferences to any number that divides 360 cleanly. This is to avoid some issues related to the math used.

Rotation Constraint Demo

Completely Fixes #1075

Holding shift while rotating a selection with the move tool
will snap the rotation to 15 degree increments
@scribblemaniac scribblemaniac added Enhancement UX Related to the way users interact with the program Transform labels Jun 18, 2019
@MrStevns
Copy link
Member

MrStevns commented Jun 30, 2019

Except for some refactoring (I've made a pull request) I think it looks good, I have two comment though, the first is about the magic number (15) that is used for hand tool. Either we should use the same preference value as movetool or it should be mentioned that only selected tools uses the preference.

My other comment is about the usability of the constraint when using the hand tool, as it doesn't respond as well as movetool. When you hold down alt and drag around, you kind of have to wriggle the mouse cursor to rotate but it's still inconsistent and in chunks.

@scribblemaniac
Copy link
Member Author

scribblemaniac commented Jul 1, 2019

The constrained rotation was not supposed to be added to the hand tool, seems like I forgot about that and did it anyway during the second commit. Unfortunately the hand tool's rotation works fundamentally differently than the move tool right now, and it makes it impossible to properly implement the rotation constraints. Basically, the move tool recalculates the full rotation based on how much the mouse has moved since pressing down, and the hand tool only calculates the partial rotation based on how much the mouse has moved since the last mouse move event. The rotation constraint must round the full rotation, not the delta, which is not currently available to the hand tool.

I will either remove the constraint support for the hand tool all together for now, or I will change the hand tool rotation if it's not too much work. It will use the preference if I get it working.

@MrStevns
Copy link
Member

MrStevns commented Jul 1, 2019

I think removing it for now seems like the best choice, then we can merge this and properly fix the problem for hand tool.

I'd like you to disregard my comment about the preference, as I didn't see that it was put under movetool. You may create a new setting for view rotation though or we could disable constrained rotation there for now.

Copy link
Member

@MrStevns MrStevns left a comment

LGTM 👍

@scribblemaniac scribblemaniac requested a review from MrStevns Jul 1, 2019
Copy link
Member

@MrStevns MrStevns left a comment

Woo documentation, well done.

@scribblemaniac
Copy link
Member Author

scribblemaniac commented Jul 1, 2019

Woo documentation, well done.

A rare sight indeed 😆 . I will merge this tomorrow if no objections come up.

@scribblemaniac scribblemaniac merged commit c2d123e into pencil2d:master Jul 2, 2019
@scribblemaniac
Copy link
Member Author

scribblemaniac commented Jul 2, 2019

Thanks @candyface for the review and improvements.

@scribblemaniac scribblemaniac deleted the constrained-rotation branch Aug 26, 2019
@Jose-Moreno Jose-Moreno added this to the 0.6.5 milestone Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Transform UX Related to the way users interact with the program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement transform tool "discrete" rotation with shortcut modifier
3 participants