Skip to content

fix: correct rotation handle positioning during vertex rotation #581

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.

Sign up for GitHub

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

Closed
HalilFocic wants to merge 1 commit into from
Closed

fix: correct rotation handle positioning during vertex rotation #581

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/core/src/view/handler/VertexHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1887,7 +1887,7 @@ class VertexHandler {
}

if (this.rotationShape) {
const alpha = toRadians(this.currentAlpha);
const alpha = toRadians(this.state.style.rotation ?? 0);
Copy link
Copy Markdown
Member

@tbouffard tbouffard Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

suggestion: Good catch! The problem was introduced during the migration from mxGraph.
Could you match the original mxGraph behavior? AFAIK, this.currentAlpha was used if not nullish. See https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/handler/mxVertexHandler.js#L2019

ℹ️ there is no need have a single commit with the initial implementation and the suggested change that would be forced pushed in the PR.
I will squash all commits when merging the PR

Copy link
Copy Markdown
Author

@HalilFocic HalilFocic Nov 22, 2024

Choose a reason for hiding this comment

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

I can try to pull it off tomorrow but if I recall correctly from code in the mxGraph currentAlpha was null by deafult, in our case its 100 for some reason. I tried setting initial alpha to 0 but the rotation was still not working. I am sure the calculation in rotation function is badly calculated when setting new alpha.

Copy link
Copy Markdown
Member

@tbouffard tbouffard Nov 22, 2024

Choose a reason for hiding this comment

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

OK I will also check on my side (I am not sure that I will be able to do it during the week-end)

Copy link
Copy Markdown
Author

@HalilFocic HalilFocic Nov 23, 2024

Choose a reason for hiding this comment

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

I looked all around this file and when declaring alpha the variable was derived from this.state.style.rotation. I logged the value of currentAlpha and this.style.rotation and they do have same values at the at but after I release there were 2 redrawHandle function calls where both style.rotation and currentAlpha was 0 (it should be 90 since i rotated for 90 deg). Confusing thing is the values are always the same but for some reason results differ. My assumption is that alpha is not set until we finish rotating while style.rotation properly updates while we rotate. This could cause this behaviour but im not sure.

const cos = Math.cos(alpha);
const sin = Math.sin(alpha);

Expand Down