Skip to content

Fix localgrid grabbing#3527

Closed
Annonnymmousss wants to merge 4 commits into
GraphiteEditor:masterfrom
Annonnymmousss:fix-localgrid-grabbing
Closed

Fix localgrid grabbing#3527
Annonnymmousss wants to merge 4 commits into
GraphiteEditor:masterfrom
Annonnymmousss:fix-localgrid-grabbing

Conversation

@Annonnymmousss
Copy link
Copy Markdown
Contributor

@Annonnymmousss Annonnymmousss commented Dec 24, 2025

fix the global state showcase when grabbed even after rotating with an angle
code todo - https://discord.com/channels/731730685944922173/881073965047636018/1399533117441835080

Screen.Recording.2025-12-24.at.6.13.43.AM.1.mov

Comment on lines +568 to +578
let viewport_delta = input.mouse.position - self.mouse_position;
let document_space_transform = self.initial_transform * document_to_viewport.inverse();
let delta_pos = if self.state.is_transforming_in_local_space {
let [local_x_axis, local_y_axis] = self.state.local_transform_axes;
let local_components = DVec2::new(viewport_delta.dot(local_x_axis), viewport_delta.dot(local_y_axis));
document_space_transform.matrix2 * local_components
} else {
document_space_transform.transform_vector2(viewport_delta)
};
let scale = document_to_viewport.y_axis.length();
let delta_scaled = (if self.slow { delta_pos / SLOWING_DIVISOR } else { delta_pos }) / scale;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Along with adding significant complexity here, you have also broken the «local x-axis constraint» feature (press G to grab then press X twice.

broken_local_grab.mp4

I don't see why there should be any changes to the functionality of the system, since the issue concerned only changing the overlays.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

letme try this again

@Annonnymmousss
Copy link
Copy Markdown
Contributor Author

this solved the constraint breakage but now every scale is getting calculated with reference to global axis only
can you help me?

@0HyperCube
Copy link
Copy Markdown
Contributor

scale is getting calculated with reference to global axis only

The scale by pressing key S then X will scale in viewport space. Pressing X again will scale in local space. This is the intended behaviour.

@Annonnymmousss
Copy link
Copy Markdown
Contributor Author

Annonnymmousss commented Dec 24, 2025

image

i meant this scale the length. It is calculated with reference to global axis(

@0HyperCube
Copy link
Copy Markdown
Contributor

0HyperCube commented Dec 24, 2025

This is the issue in #3341 that you are trying to solve in this PR? I am confused since you linked an unrelated discord post…

I assume you are referring to the blue numbers in your screenshot. These are drawn by the overlay_context.translation_box function. If you look at the implementation, you will see that it uses the first argument, translation, to determine the numbers that are drawn. To change these numbers, you can transform the vector:

// A transformation to the local space of the transformation from the document
let document_to_local = glam::DMat2::from_cols(self.state.local_transform_axes[0], self.state.local_transform_axes[1]).inverse();
let transform_local = document_to_local * document_to_viewport.matrix2.inverse() * translation_viewport;
overlay_context.translation_box(transform_local, quad, typed_string);

@Keavon Keavon marked this pull request as draft December 25, 2025 00:30
@Annonnymmousss
Copy link
Copy Markdown
Contributor Author

yeah sorry for the confusion created due to terminologies ill keep this in mind for my further contributions.
and ill be working on it..

@Annonnymmousss
Copy link
Copy Markdown
Contributor Author

Screen.Recording.2025-12-25.at.6.37.00.PM.1.mov

I have made some changes i guess it fixes the issue. Please review it and let me know if any breakage is caused or any suggestions.

@Annonnymmousss Annonnymmousss marked this pull request as ready for review December 25, 2025 13:15
Comment on lines +253 to +254
let document_to_local = glam::DMat2::from_cols(self.state.local_transform_axes[0], self.state.local_transform_axes[1]).inverse();
let transform_local = document_to_local * document_to_viewport.matrix2.inverse() * translation_viewport;
Copy link
Copy Markdown
Contributor

@0HyperCube 0HyperCube Dec 25, 2025

Choose a reason for hiding this comment

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

Suggested change
let document_to_local = glam::DMat2::from_cols(self.state.local_transform_axes[0], self.state.local_transform_axes[1]).inverse();
let transform_local = document_to_local * document_to_viewport.matrix2.inverse() * translation_viewport;
let viewport_to_local = glam::DMat2::from_cols(self.state.local_transform_axes[0], self.state.local_transform_axes[1]).inverse();
let transform_local = viewport_to_local * translation_viewport / document_to_viewport.matrix2.y_axis.length();

I scuffed up the transformations sorry. The local_transform_axes are actually in viewport space.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ya i will update these changes

Copy link
Copy Markdown
Contributor

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

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

Looks good to me; thanks for your work on these changes.

I'm a bit unsure as to if the local space should be shown when transforming on the global X-axis (G then X once). It also breaks the measuring number in that case when you type some digits to choose an exact translation. This is because the exact typed number is used to avoid rounding errors.

@Annonnymmousss
Copy link
Copy Markdown
Contributor Author

Annonnymmousss commented Dec 25, 2025

okay if that would be the case I will open another PR.

@Annonnymmousss
Copy link
Copy Markdown
Contributor Author

@Keavon is it ready for merge or any changes are required?

@Ayush2k02
Copy link
Copy Markdown
Contributor

LGTM !

@Keavon Keavon force-pushed the fix-localgrid-grabbing branch from 354c819 to 49b9cd8 Compare February 16, 2026 22:24
@Keavon
Copy link
Copy Markdown
Member

Keavon commented Feb 16, 2026

Finally reviewing this, sorry for the wait.

From what I am seeing, this makes some changes related to what is desired but does not actually fix the exact issue. I'm a bit confused looking at the PR description and seeing what looks like the wrong #code-todo-list Discord message linked, or maybe something got mixed up there between what I posted and what got interpreted.

The only desired change that I know of is the need to show the translation in local space upon hitting the second occurrence of X to go from unconstrained to X-constrained to X-local-constrained space, where it currently still shows the X and Y components in global space rather than the single X component in local space:

capture_21_.mp4

And then as we see at the end, if I type 100, it incorrectly shows "100" in both the X and Y components, which further shows why this is wrong and we need it to show that in local space. The equivalent of this for S works flawlessly:

capture_22_.mp4

I actually think the best option is to close this and let you reference this implementation to fix the problem described herein, since the current PR relates to this but (undesirably) changes other functionality while not changing this desired issue. So starting over, but using this code for reference, seems simplest to me. Feel free to reference this comment in the new PR. Thanks for your understanding.

@Keavon Keavon closed this Feb 16, 2026
@Keavon
Copy link
Copy Markdown
Member

Keavon commented Feb 16, 2026

That will be issue #3341.

@0HyperCube
Copy link
Copy Markdown
Contributor

@Keavon I don't think you have tested the correct branch. I have included (for your reference) the current behaviour of this branch (commit 49b9cd8) when pressing G then X twice then typing 100.

local_grab.mp4

It is somewhat disappointing to see that you cannot even test the correct commit after two months.

@Keavon
Copy link
Copy Markdown
Member

Keavon commented Feb 17, 2026

To be clear, my videos are from master, not this branch. They are part of describing the desired behavior, which after writing about, I discovered has an existing description in #3341.

@0HyperCube
Copy link
Copy Markdown
Contributor

My sincerest apologies for this misunderstanding. I was attempting to demonstrate that (as far as I can tell) the desired behaviour has been correctly implemented. As per my video above, the local axis grabbing shows in the local space of the layer and typing values for the translation shows in local space.

Therefore I struggle to comprehend why you have said that this PR "changes other functionality while not changing this desired issue".

I'm sorry for my ignorance on this matter and will refrain from commenting further.

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.

4 participants