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

Canvas: Add ability to rotate elements #83295

Merged
merged 17 commits into from Mar 22, 2024
Merged

Canvas: Add ability to rotate elements #83295

merged 17 commits into from Mar 22, 2024

Conversation

nmarrs
Copy link
Contributor

@nmarrs nmarrs commented Feb 23, 2024

More relatively low hanging fruit similar to #80407

Add ability to rotate canvas elements via the rotatable functionality that already exists inside the moveable library. This was previously implemented temporarily for the cancad hackathon project

Screen.Recording.2024-03-21.at.1.51.35.PM.mov

TODO

  • Fix strange resizing / reset of rotation
  • Test (all element types)
  • Improve handling of hiding custom ables
  • merge (profit)

Known issues / next steps

@nmarrs nmarrs added add to changelog area/panel/canvas Issues related to canvas panel no-backport Skip backport of PR add to what's new labels Feb 23, 2024
@nmarrs nmarrs added this to the 11.0.x milestone Feb 23, 2024
@nmarrs nmarrs self-assigned this Feb 23, 2024
@lukasztyrala
Copy link
Member

Few comments:

  • we should keep the cursor consistent when rotating
  • it seems that moving an object will reset its rotation – I think it is not an anticipated behavior, as here the system, without warning, overwrites the previous intentional interaction by the user
  • can we rotate by a fixed radius that would allow people to rotate by 90° or 45° easily? Or snap to certain positions (like 0°, 45°, 90°, …) – free rotation precision is tough to achieve with a mouse :-)

@nmarrs nmarrs changed the title [POC] [WIP] Canvas: Add ability to rotate elements Canvas: Add ability to rotate elements Mar 21, 2024
@nmarrs nmarrs marked this pull request as ready for review March 21, 2024 21:23
@nmarrs nmarrs requested a review from a team as a code owner March 21, 2024 21:23
@nmarrs nmarrs requested review from codeincarnate and baldm0mma and removed request for a team March 21, 2024 21:23
…tated elements (not perfect but "good enough" for time being)
@@ -408,6 +424,10 @@ export class Scene {
draggable: allowChanges && !this.editModeEnabled.getValue(),
resizable: allowChanges,

// Setup rotatable
rotatable: allowChanges,
throttleRotate: 5,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in effect our version of angle "snapping" making it easier to get to 45, 90, 180 etc values without explicitly setting in the placement editor

const originalDirTB = dirTB;

dirLR = Math.sign(originalDirLR * Math.cos(rotationInRadians) - originalDirTB * Math.sin(rotationInRadians));
dirTB = Math.sign(originalDirLR * Math.sin(rotationInRadians) + originalDirTB * Math.cos(rotationInRadians));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and you thought you were done with geometry in school? 🎒

Copy link
Contributor

Choose a reason for hiding this comment

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

SOHCAHTOA!!!!

Copy link
Collaborator

@codeincarnate codeincarnate left a comment

Choose a reason for hiding this comment

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

Tested out in a number of different scenarios and seems to be working well. Good stuff! 😄

Copy link
Contributor

@baldm0mma baldm0mma left a comment

Choose a reason for hiding this comment

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

Nice! Works as expected! I will say the snapping to other elements, especially if there are A LOT of elements feels a little jarring. But that's maybe not something we can easily change, since it is the expected behavior.

Screen.Recording.2024-03-22.at.1.40.52.PM.mov

Comment on lines +396 to +401
enableCustomables = () => {
this.moveable!.props = {
dimensionViewable: true,
constraintViewable: true,
settingsViewable: true,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

+100

@nmarrs nmarrs merged commit 566cee7 into main Mar 22, 2024
19 checks passed
@nmarrs nmarrs deleted the canvas-rotatable-poc branch March 22, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants