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.

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

Add test for chained GRS transformations #2475

Merged
merged 8 commits into from
Mar 29, 2025
Merged

Add test for chained GRS transformations #2475

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3a466ea
added tests for chained tasks
rahat2134 Mar 23, 2025
04d295c
added tests for chained tasks, applied the feedback from 0hypercube
rahat2134 Mar 25, 2025
feec990
Merge branch 'master' into grs-chained-tests
rahat2134 Mar 26, 2025
4069a8b
Merge branch 'master' into grs-chained-tests
rahat2134 Mar 27, 2025
f3b7a3b
some changes in grs chained test to make it more robust
rahat2134 Mar 29, 2025
dbbd9d6
Merge branch 'master' of https://github.com/GraphiteEditor/Graphite i…
rahat2134 Mar 29, 2025
6fe5f5a
Merge branch 'grs-chained-tests' of https://github.com/rahat2134/Grap…
rahat2134 Mar 29, 2025
12fdb2c
unnecessary changes
rahat2134 Mar 29, 2025
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
74 changes: 74 additions & 0 deletions editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs
View file
Original file line number Diff line number Diff line change
@@ -908,4 +908,78 @@ mod test_transform_layer {
let translation_diff = (after_cancel.translation - original_transform.translation).length();
assert!(translation_diff < 1.0, "Translation component changed too much: {}", translation_diff);
}

#[tokio::test]
async fn test_grab_rotate_scale_chained() {
let mut editor = EditorTestUtils::create();
editor.new_document().await;
editor.drag_tool(ToolType::Rectangle, 0., 0., 100., 100., ModifierKeys::empty()).await;
let document = editor.active_document();
let layer = document.metadata().all_layers().next().unwrap();
editor.handle_message(NodeGraphMessage::SelectedNodesSet { nodes: vec![layer.to_node()] }).await;
let original_transform = get_layer_transform(&mut editor, layer).await.unwrap();

editor.handle_message(TransformLayerMessage::BeginGrab).await;
editor.move_mouse(150.0, 130.0, ModifierKeys::empty(), MouseKeys::NONE).await;
editor
.handle_message(TransformLayerMessage::PointerMove {
slow_key: Key::Shift,
increments_key: Key::Control,
})
.await;

let after_grab_transform = get_layer_transform(&mut editor, layer).await.unwrap();
let expected_translation = DVec2::new(50.0, 30.0);
let actual_translation = after_grab_transform.translation - original_transform.translation;
assert!(
(actual_translation - expected_translation).length() < 1e-5,
"Expected translation of {:?}, got {:?}",
expected_translation,
actual_translation
);

// 2. Chain to rotation - from current position to create ~45 degree rotation
editor.handle_message(TransformLayerMessage::BeginRotate).await;
editor.move_mouse(190.0, 90.0, ModifierKeys::empty(), MouseKeys::NONE).await;
editor
.handle_message(TransformLayerMessage::PointerMove {
slow_key: Key::Shift,
increments_key: Key::Control,
})
.await;
let after_rotate_transform = get_layer_transform(&mut editor, layer).await.unwrap();
// Checking for off-diagonal elements close to 0.707, which corresponds to cos(45°) and sin(45°)
assert!(
!after_rotate_transform.matrix2.abs_diff_eq(after_grab_transform.matrix2, 1e-5) &&
(after_rotate_transform.matrix2.x_axis.y.abs() - 0.707).abs() < 0.1 && // Check for off-diagonal elements close to 0.707
(after_rotate_transform.matrix2.y_axis.x.abs() - 0.707).abs() < 0.1, // that would indicate ~45° rotation
"Rotation should change matrix components with approximately 45° rotation"
);

// 3. Chain to scaling - scale(area) up by 2x
editor.handle_message(TransformLayerMessage::BeginScale).await;
editor.move_mouse(250.0, 200.0, ModifierKeys::empty(), MouseKeys::NONE).await;
editor
.handle_message(TransformLayerMessage::PointerMove {
slow_key: Key::Shift,
increments_key: Key::Control,
})
.await;

let after_scale_transform = get_layer_transform(&mut editor, layer).await.unwrap();
let before_scale_det = after_rotate_transform.matrix2.determinant();
let after_scale_det = after_scale_transform.matrix2.determinant();
assert!(
after_scale_det >= 2.0 * before_scale_det,
"Scale should increase the determinant of the matrix (before: {}, after: {})",
before_scale_det,
after_scale_det
);

editor.handle_message(TransformLayerMessage::ApplyTransformOperation { final_transform: true }).await;
let final_transform = get_layer_transform(&mut editor, layer).await.unwrap();

assert!(final_transform.abs_diff_eq(after_scale_transform, 1e-5), "Final transform should match the transform before committing");
assert!(!final_transform.abs_diff_eq(original_transform, 1e-5), "Final transform should be different from original transform");
Copy link
Member

Choose a reason for hiding this comment

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

Verifying that the transform is different is not really good enough. We are trying to test that the pivots and transformations all work correctly, which was fixed in #2450.

I would expect you to build a transform based on the expected scaling and translation and then to assert that it is equal to the actual transform of the layer. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0HyperCube I'm having difficulty with your suggestion to "build a transform based on the expected scaling and translation and then assert that it is equal to the actual transform".

What I've tried:

  • Tried to carefully calculate expected transforms with pivot points:
// Pivot point handling
let pivot_to_origin = DAffine2::from_translation(-pivot);
let origin_to_pivot = DAffine2::from_translation(pivot);

// Transform components
let expected_translate = DAffine2::from_translation(translation_offset);
let rotation = DAffine2::from_angle(rotation_angle);
let expected_rotate = origin_to_pivot * rotation * pivot_to_origin;
let scaling = DAffine2::from_scale(DVec2::splat(scale_factor));
let expected_scale = origin_to_pivot * scaling * pivot_to_origin;

// Combined transform
let expected_transform = expected_scale * expected_rotate * expected_translate;
  • This still gave significantly different results from the actual transform

For example, moving to (20,30) resulted in:

After grab: { matrix2: DMat2 { x_axis: DVec2(1.0, 0.0), y_axis: DVec2(0.0, 1.0) }, translation: DVec2(-30.0, -20.0) }

Could you point me to the specific transformation logic in the codebase I should use to calculate the expected transform matrix?

Or

If i am doing something wrong!

Copy link
Member

Choose a reason for hiding this comment

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

@rahat2134 You where presumably moving the mouse from (50,50) to (20,30) in order to get a translation on the object of (-30, -20)?

Copy link
Member

Choose a reason for hiding this comment

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

The approach that you had at the start seems reasonable. However you will obviously need to ensure you have the correct pivot position.

Copy link
Contributor Author

@rahat2134 rahat2134 Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @0HyperCube Does this make any sense, Here we are checking expected scaling and translation?

	#[tokio::test]
	async fn test_grab_rotate_scale_chained() {
		let mut editor = EditorTestUtils::create();
		editor.new_document().await;
		editor.drag_tool(ToolType::Rectangle, 0., 0., 100., 100., ModifierKeys::empty()).await;

		let document = editor.active_document();
		let layer = document.metadata().all_layers().next().unwrap();
		editor.handle_message(NodeGraphMessage::SelectedNodesSet { nodes: vec![layer.to_node()] }).await;

		let original_transform = get_layer_transform(&mut editor, layer).await.unwrap();

		// 1. Grab operation - move from center position to a new position
		let start_pos = DVec2::new(50.0, 50.0);
		let grab_pos = DVec2::new(80.0, 70.0);
		// These coordinates appear to be transformed with:
		// 1. Inversion of sign
		// 2. Swap of x/y (or some other transformation)
		let expected_translation = DVec2::new(-20.0, -30.0);

		editor.handle_message(TransformLayerMessage::BeginGrab).await;
		editor.move_mouse(grab_pos.x, grab_pos.y, ModifierKeys::empty(), MouseKeys::NONE).await;
		editor
			.handle_message(TransformLayerMessage::PointerMove {
				slow_key: Key::Shift,
				increments_key: Key::Control,
			})
			.await;

		let after_grab_transform = get_layer_transform(&mut editor, layer).await.unwrap();
		let actual_translation = after_grab_transform.translation - original_transform.translation;

		assert!(
			(actual_translation - expected_translation).length() < 1e-5,
			"Expected translation near {:?}, got {:?}",
			expected_translation,
			actual_translation
		);

		editor.handle_message(TransformLayerMessage::BeginRotate).await;
		editor.move_mouse(150.0, 50.0, ModifierKeys::empty(), MouseKeys::NONE).await;
		editor
			.handle_message(TransformLayerMessage::PointerMove {
				slow_key: Key::Shift,
				increments_key: Key::Control,
			})
			.await;

		let after_rotate_transform = get_layer_transform(&mut editor, layer).await.unwrap();
		assert!(!after_rotate_transform.abs_diff_eq(after_grab_transform, 1e-5), "Rotation should change the transform after grab");
		// Verifying rotation matrix components have changed for more robustness
		assert!(
			!after_rotate_transform.matrix2.abs_diff_eq(after_grab_transform.matrix2, 1e-5),
			"Rotation should change matrix components"
		);

		// 3. Scale operation
		editor.handle_message(TransformLayerMessage::BeginScale).await;
		// Move to position that would create ~2x scale
		let scale_target = DVec2::new(grab_pos.x + 100.0, grab_pos.y + 100.0);
		editor.move_mouse(scale_target.x, scale_target.y, ModifierKeys::empty(), MouseKeys::NONE).await;
		editor
			.handle_message(TransformLayerMessage::PointerMove {
				slow_key: Key::Shift,
				increments_key: Key::Control,
			})
			.await;

		let after_scale_transform = get_layer_transform(&mut editor, layer).await.unwrap();
		// Expected scale factor (approximately 2.0)
		let expected_scale_factor = 2.0;
		let actual_scale_factor = (after_scale_transform.matrix2.determinant() / after_rotate_transform.matrix2.determinant()).sqrt();

		assert!(
			(actual_scale_factor - expected_scale_factor).abs() < 0.5,
			"Expected scale factor near {}, got {}",
			expected_scale_factor,
			actual_scale_factor
		);

		editor.handle_message(TransformLayerMessage::ApplyTransformOperation { final_transform: true }).await;
		
		// Final check is still remaining
	}

Copy link
Contributor Author

@rahat2134 rahat2134 Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If I am going in right way then what do you want me to check finally after committing (please tell elaborately)? i.e after editor.handle_message(TransformLayerMessage::ApplyTransformOperation { final_transform: true }).await;

if not going in right way then also give some suggestions or guidance!

Thank you so much!

Copy link
Member

Choose a reason for hiding this comment

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

Please just update the PR instead of putting an 80 line code block.

I'm extremly confused by:

// These coordinates appear to be transformed with:
// 1. Inversion of sign
// 2. Swap of x/y (or some other transformation)

Please note that the editor.move_mouse function moves the mouse from whatever position it was at previously to the position specified. Since the last operation was to draw a rectangle to (100,100), the mouse will therefore be at (100,100). Moving it to (80.0, 70.0) will clearly mean a transform of (-20, -30).

Also please can you use the DAffine2::abs_diff_eq function as previously requested. This ensures that all components of the transform are correct.

Thanks again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it again, Please have a look!

}
}
Loading
Oops, something went wrong.