Nested layer transforms API #301

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

Jump to bottom

Merged

TrueDoctor merged 53 commits into master from transform-api

Aug 6, 2021

Member

TrueDoctor commented Jul 24, 2021 •

edited

Loading

Fixes #251
unblocks #149
Fixes #148
Fixes #290

This change is

Keavon changed the title ~~Transform api~~ Nested layer transforms API

cloudflare-workers-and-pages bot commented Jul 24, 2021 •

edited

Loading

Deploying with Cloudflare Pages

Latest commit:	`83a1a4b`
Status:	✅ Deploy successful!
Preview URL:	https://af316449.graphite-master.pages.dev

TrueDoctor force-pushed the transform-api branch 6 times, most recently from 5959ed8 to 407fbd9 Compare

August 3, 2021 21:23

henryksloan reviewed

View reviewed changes

core/editor/src/document/document_file.rs Outdated Show resolved Hide resolved

TrueDoctor force-pushed the transform-api branch from b779b24 to bd4b1e2 Compare

August 4, 2021 16:21

TrueDoctor requested review from 0HyperCube, henryksloan, pkupper and Keavon

August 4, 2021 16:49

TrueDoctor marked this pull request as ready for review

August 4, 2021 16:49

henryksloan reviewed

View reviewed changes

client/web/src/components/panels/LayerTree.vue Outdated Show resolved Hide resolved

core/document/src/intersection.rs Show resolved Hide resolved

core/document/src/intersection.rs Outdated Show resolved Hide resolved

core/document/src/intersection.rs Show resolved Hide resolved

core/document/src/layers/folder.rs Outdated Show resolved Hide resolved

core/editor/src/document/layer_panel.rs

+              			scale: 1.,
+              		}
+              	}
+              	pub fn snapped_angle(&self) -> f64 {

Contributor

henryksloan Aug 5, 2021

I think we should be sure to always have blank lines between functions in impl blocks. Assuming you agree, check for this elsewhere in this CL.

core/editor/src/document/layer_panel.rs Show resolved Hide resolved

core/editor/src/document/layer_panel.rs

Comment on lines +49 to +55

+              	if !layer_data.contains_key(path) {
+              		layer_data.insert(path.to_vec(), LayerData::new(false));
+              	}
+              	layer_data.get_mut(path).unwrap()

Contributor

henryksloan Aug 5, 2021

I believe this can be achieved in one line... something like get_or_insert_with.

core/editor/src/document/layer_panel.rs Outdated Show resolved Hide resolved

core/editor/src/tool/tools/eyedropper.rs

-              						ToolMessage::Eyedropper(EyedropperMessage::RightMouseDown) => responses.push_back(ToolMessage::SelectSecondaryColor(color).into()),
-              						_ => {}
-              					}
+              				if let LayerDataType::Shape(s) = &layer.data {

Contributor

henryksloan Aug 5, 2021

I'm not a fan of replacing and_then with nested maps - and_then is designed to achieve exactly this.

TrueDoctor commented

View reviewed changes

Member Author

TrueDoctor left a comment

Reviewable status: 0 of 54 files reviewed, 12 unresolved discussions (waiting on @0HyperCube, @henryksloan, @Keavon, @pkupper, and @TrueDoctor)

client/web/src/components/panels/LayerTree.vue, line 333 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

Split this into multiple lines?

Done.

core/document/src/intersection.rs, line 14 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

Add a blank line before this function signature

Done.

core/document/src/intersection.rs, line 44 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

I think this for and if could be refactored to just an if using Iterator::any

Done.

core/document/src/intersection.rs, line 57 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

This can also be refactored with Iterator::any

That would get pretty ugly if I don't want to loop through the iterator multiple times

core/document/src/layers/folder.rs, line 33 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

Consider splitting this across multiple lines

Done.

core/document/src/layers/simple_shape.rs, line 87 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

Parentheses around ((sides + 1) % 2) as f64 * PI would be nice, to be clear about the order of operations for the division.

Done.

core/editor/src/document/document_file.rs, line 86 at r1 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

I believe this message is removed in master, replaced by FlipSelectedLayers. Also, FlipSelectedLayers should probably be down here, next to the other transformations of the selected layers.

Done.

core/editor/src/document/layer_panel.rs, line 43 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

I'm not sure these variables need transform in their names; that makes it much more verbose, and I'm not sure it makes it clearer.

Just copied this over and found out that it is fundamentally broken and needs a fix anyway
Will add a todo

core/editor/src/document/layer_panel.rs, line 52 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

I believe this can be achieved in one line... something like get_or_insert_with.

But then you need a owned path and can't clone on demand

core/editor/src/document/layer_panel.rs, line 58 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

If these variables are only used to populate the struct, I don't think they need to be stored in variables at all.

Done.

core/editor/src/tool/tools/eyedropper.rs, line 26 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

I'm not a fan of replacing and_then with nested maps - and_then is designed to achieve exactly this.

Done.

TrueDoctor commented

View reviewed changes

Member Author

TrueDoctor left a comment

Reviewable status: 0 of 54 files reviewed, 12 unresolved discussions (waiting on @0HyperCube, @henryksloan, @Keavon, and @pkupper)

core/editor/src/document/layer_panel.rs, line 31 at r3 (raw file):

Previously, henryksloan (Henry Sloan) wrote…

I think we should be sure to always have blank lines between functions in impl blocks. Assuming you agree, check for this elsewhere in this CL.

We'll try to add them as we got through the pr

core/editor/src/document/layer_panel.rs, line 43 at r3 (raw file):

Previously, TrueDoctor wrote…

Just copied this over and found out that it is fundamentally broken and needs a fix anyway
Will add a todo

Done.

TrueDoctor added 12 commits

August 5, 2021 08:05


          Enforce cache dirtification

96d6581


          Test stuff out

149a5c8


          Turn all shapes into one struct

d9db870


          Rework render function

4997d11


          Renaming stuff + debugging

a42501d


          Remove working folder

6edffbc


          Remove old shapes

58c71e2


          Fix layer tree

20b2dfe


          Major restructuring

74cd1d8


          Plumb up action flow

651ab80


          Run cargo fix

b637b2b


          Make tests compile

f569910

TrueDoctor added 9 commits

August 5, 2021 08:05


          Remove duplicate scale matrix

ff76b5f


          Fix format

b48ae80


          Fix tests

6cb986e


          Do less stupid stuff

b8b808f


          Fix issue in scoped transform

1fb1f56


          Refactor tools to avoid state keeping

2245c65


          Refactor more tools to use state that is passed along

a93ee8c


          Macro magic

8080c2c


          Add review fixes

c2f7637

TrueDoctor force-pushed the transform-api branch from fdcf8dc to c2f7637 Compare

August 5, 2021 06:05

TrueDoctor added 3 commits

August 5, 2021 11:30


          Apply further review suggestions

3ad5bb1


          Update thumbnail on document rotation

d4b0c48


          Minor cleanups

0a38058

Keavon approved these changes

View reviewed changes

core/editor/src/tool/tools/ellipse.rs Outdated Show resolved Hide resolved

core/editor/src/tool/tools/ellipse.rs Outdated Show resolved Hide resolved

core/editor/src/tool/tools/line.rs Outdated Show resolved Hide resolved

core/editor/src/tool/tools/rectangle.rs Outdated Show resolved Hide resolved

core/editor/src/tool/tools/select.rs Outdated Show resolved Hide resolved

core/editor/src/tool/tools/select.rs Outdated Show resolved Hide resolved

core/editor/src/tool/tools/shape.rs Outdated Show resolved Hide resolved

core/editor/src/consts.rs Outdated Show resolved Hide resolved

TrueDoctor added 2 commits

August 6, 2021 09:15


          Fix line tool behaviour

39628b3


          Fix whitespace + change selection box style

9ee90b6

TrueDoctor commented

View reviewed changes

Member Author

TrueDoctor left a comment

Reviewable status: 0 of 55 files reviewed, 20 unresolved discussions (waiting on @0HyperCube, @henryksloan, @Keavon, and @pkupper)

core/editor/src/consts.rs, line 14 at r5 (raw file):

Previously, Keavon (Keavon Chambers) wrote…

Wondering if this should be 15. However this already was 12 beforehand, yet the actual application does work in 15° increments while holding Shift.

Done.

core/editor/src/tool/tools/ellipse.rs, line 33 at r5 (raw file):

Previously, Keavon (Keavon Chambers) wrote…

Two spaces between DragStop, and Abort,

Done.

core/editor/src/tool/tools/ellipse.rs, line 98 at r5 (raw file):

Previously, Keavon (Keavon Chambers) wrote…

Is this comment still accurate? Also please switch from TODO - to TODO: for consistency (and because it helps with tooling which highlights TODO: blocks but apparently mine doesn't highlight TODO - ).

Yes that is still a thing we should do, should be a pretty small fix

core/editor/src/tool/tools/line.rs, line 33 at r5 (raw file):

Previously, Keavon (Keavon Chambers) wrote…

Remove double space and space before closing paren.

Done.

core/editor/src/tool/tools/rectangle.rs, line 33 at r5 (raw file):

Previously, Keavon (Keavon Chambers) wrote…

Double space before Abort,.

Done.

core/editor/src/tool/tools/select.rs, line 45 at r5 (raw file):

Previously, Keavon (Keavon Chambers) wrote…

Double space here also.

Done.

core/editor/src/tool/tools/select.rs, line 130 at r5 (raw file):

Previously, Keavon (Keavon Chambers) wrote…

Let's make the stroke width 1.0 and the hex code #00A8FF (0x00, 0xA8, 0xFF).

Done.

core/editor/src/tool/tools/shape.rs, line 33 at r5 (raw file):

Previously, Keavon (Keavon Chambers) wrote…

Double space here too.

Good thing I would never copy paste code, pretty weird chance that the duplicate whitespace was in all files!

TrueDoctor added 9 commits

August 6, 2021 09:31


          Fix line with locked angle

cae5085


          Fix rearranging layers not updating the thumbnail

37aa628


          "Fixed" thumbnail rendering

ab3c0cb


          Fix Line angle snapping

56eb004


          Remove unneded dirtification upon root rotation

5180a67


          Set viewbox of svg export based on the contents

4896a05


          Add margins for document export

554ce05


          Remove margins

22437b0


          final fix

83a1a4b

TrueDoctor merged commit f79e6e3 into master

TrueDoctor deleted the transform-api branch

August 6, 2021 10:34

Keavon pushed a commit that referenced this pull request


          Transform API (#301)

da290f1

* Enforce cache dirtification

* Turn all shapes into one struct

* Remove working folder

* Remove old shapes

* Major restructuring

* Refactor Ellipse, Rectangle and ShapeTool

* Simplify bounding box calculation for folder

* Fix panic in select tool

*  Refactorselect tool

* Refactor Align

* Refactor flipping layers

* Zoom to fit all

* Refactor tools to avoid state keeping

* Refactor more tools to use state that is passed along

* Fix whitespace + change selection box style

* Set viewbox of svg export based on the contents

Keavon pushed a commit that referenced this pull request


          Transform API (#301)

699562b

* Enforce cache dirtification

* Turn all shapes into one struct

* Remove working folder

* Remove old shapes

* Major restructuring

* Refactor Ellipse, Rectangle and ShapeTool

* Simplify bounding box calculation for folder

* Fix panic in select tool

*  Refactorselect tool

* Refactor Align

* Refactor flipping layers

* Zoom to fit all

* Refactor tools to avoid state keeping

* Refactor more tools to use state that is passed along

* Fix whitespace + change selection box style

* Set viewbox of svg export based on the contents

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet