Rotation first steps #66

Xavman42 · 2022-09-25T20:39:44Z

Added transform_origin field to interface classes.

Attempted to test the field but ran into some issues. The relevant code is commented out in path_interface.py and test_text_interface.py.

Things should be formatted correctly now.

ajyoon

This is a good start! The tests aren't working because you're missing the code which actually plugs the new values into the underlying Qt objects. For example, TextInterface._get_path should pass the transform origin into QClippingPath, and QClippingPath should call self.setTransformOriginPoint in its init (like how it does for self.setRotation). Similarly, ImageInterface._apply_common_properties should apply the transform origin values like it does for rotation and scale.

ajyoon · 2022-09-26T19:12:35Z

tests/test_interface/test_text_interface.py

+    #       text = TextInterface(
+    #           ORIGIN, self.brush, self.pen, "foo", self.font, rotation=(Unit(12),Unit(12))
+    #       )
+    #       assert text._create_qt_object().transform_origin() == (Unit(12),Unit(12))


Qt points are expressed as QPointF values. neoscore.interface.qt.converters has some convenience methods for converting neoscore points to Qt points.

ajyoon · 2022-09-26T19:12:50Z

tests/test_interface/test_text_interface.py

@@ -55,6 +55,14 @@ def test_rotation(self):
        )
        assert text._create_qt_object().rotation() == 123

+    #   def test_rotation_about_transform_origin(self):
+    #       text = TextInterface(ORIGIN, self.brush, self.pen, "foo", self.font)
+    #       assert text._create_qt_object().transform_origin() == ORIGIN


The Qt field is called transformOriginPoint

ajyoon · 2022-09-26T19:13:24Z

tests/test_interface/test_text_interface.py

+    #       text = TextInterface(ORIGIN, self.brush, self.pen, "foo", self.font)
+    #       assert text._create_qt_object().transform_origin() == ORIGIN
+    #       text = TextInterface(
+    #           ORIGIN, self.brush, self.pen, "foo", self.font, rotation=(Unit(12),Unit(12))


I think you mean to set transform_origin=(Unit(12), Unit(12)) here

ajyoon · 2022-09-26T19:14:23Z

neoscore/interface/image_interface.py

@@ -36,6 +38,9 @@ class ImageInterface(PositionedObjectInterface):
    z_index: int = 0
    """Z-index controlling draw order."""

+    transform_origin: Point = ORIGIN
+    """Axis of rotation"""


This isn't just the rotation axis, it's the axis for all transformations. For our purposes, that means scaling and rotation. A better docstring would be something like "The origin point for rotation and scaling transforms"

Xavman42 · 2022-09-27T00:45:14Z

Made some progress today in understanding how all this works. I still have not figured out how to add transform_origin to QClippingPath. Once I figure that out, I should be able to un-comment line 121 in text_interface.py and subsequently un-comment line 72 in test_text_interface.py and actually pass the test.

ajyoon

this should work! you just need to apply the described change. this also needs unit tests for all the other interface classes with the new field, not just TextInterface

ajyoon · 2022-09-27T18:26:07Z

neoscore/interface/path_interface.py

@@ -123,6 +128,7 @@ def _create_qt_object(self) -> QClippingPath:
            self.rotation,
            self.background_brush.qt_object if self.background_brush else None,
            defer_geometry_calculation=True,
+            # self.transform_origin,


You just need to uncomment this line and bind it

Suggested change

# self.transform_origin,

transform_origin=self.transform_origin,

ajyoon · 2022-09-27T18:27:15Z

tests/test_interface/test_text_interface.py

@@ -55,6 +56,21 @@ def test_rotation(self):
        )
        assert text._create_qt_object().rotation() == 123

+    def test_transformOriginPoint(self):


this name should be test_transform_origin_point (following typical python naming conventions, where snake case is used for most things)

ajyoon · 2022-09-27T18:33:57Z

also, please run the cleanup scripts before committing, as I see the import cleanup wasn't run. I recommend doing this in a pre-commit hook so you can't forget. check out the guide for this.

(we temporarily disabled the build check on imports, but I'm going to re-enable it soon)

Xavman42 · 2022-09-30T16:33:56Z

Thanks for the assistance!

Tests should all be made, and they are all passing.
I'm on a Windows machine currently using WSL to get access to bash commands. I can install the devtools just fine, but I may be getting an error when applying the pre_commit hook, I'm not sure...

sh dev_scripts/pre_commit.sh 
: not found/pre_commit.sh: 2: 
Running pre-commit cleanup
: not found/pre_commit.sh: 4: 
dev_scripts/pre_commit.sh: 5: set: Illegal option -

Otherwise, Black seems to handle a lot of the formatting just fine.

ajyoon · 2022-09-30T19:26:48Z

Hm yeah seems like some WSL integration issue. The step that seems to not be running on your end is isort, which you can run manually with isort .. It's easy though so I just ran it myself and pushed. Once the build passes I will merge. I'll update the issue with next steps. thank you!

ajyoon · 2022-09-30T19:36:58Z

btw, for future PRs I recommend creating your own separate branch off of origin/rotation. I just squash-merged this PR into origin/main, so you'll notice your local branch is now out of sync with origin/rotation. It's best practice to create a separate branch for each PR and delete it after merging.

Xavman42 added 2 commits September 25, 2022 15:18

test commit

625748d

formatting update

a0d61aa

ajyoon reviewed Sep 26, 2022

View reviewed changes

Incremental progress

291a53b

ajyoon reviewed Sep 27, 2022

View reviewed changes

All tests successful

4175139

sort imports

ae53b9b

ajyoon merged commit b9879d5 into DigiScore:rotation Sep 30, 2022

Xavman42 deleted the origin/rotation branch September 30, 2022 20:55

Xavman42 restored the origin/rotation branch September 30, 2022 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rotation first steps #66

Rotation first steps #66

Xavman42 commented Sep 25, 2022

ajyoon left a comment

ajyoon Sep 26, 2022

ajyoon Sep 26, 2022

ajyoon Sep 26, 2022

ajyoon Sep 26, 2022

Xavman42 commented Sep 27, 2022

ajyoon left a comment

ajyoon Sep 27, 2022

ajyoon Sep 27, 2022

ajyoon commented Sep 27, 2022

Xavman42 commented Sep 30, 2022

ajyoon commented Sep 30, 2022

ajyoon commented Sep 30, 2022

	# self.transform_origin,
	transform_origin=self.transform_origin,

Rotation first steps #66

Rotation first steps #66

Conversation

Xavman42 commented Sep 25, 2022

ajyoon left a comment

Choose a reason for hiding this comment

ajyoon Sep 26, 2022

Choose a reason for hiding this comment

ajyoon Sep 26, 2022

Choose a reason for hiding this comment

ajyoon Sep 26, 2022

Choose a reason for hiding this comment

ajyoon Sep 26, 2022

Choose a reason for hiding this comment

Xavman42 commented Sep 27, 2022

ajyoon left a comment

Choose a reason for hiding this comment

ajyoon Sep 27, 2022

Choose a reason for hiding this comment

ajyoon Sep 27, 2022

Choose a reason for hiding this comment

ajyoon commented Sep 27, 2022

Xavman42 commented Sep 30, 2022

ajyoon commented Sep 30, 2022

ajyoon commented Sep 30, 2022