Add a movable canvas with matricies #175

0HyperCube · 2021-06-10T17:31:24Z

Convert rectangles (kurbo::Rect is axis aligned so cannot be rotated / skewed) and shape (regular polygons which previously used the ShapePoints struct) to use the kurbo::BezPath struct
Add Glam
Use matrix transform for operations
Add Document Matrix
Allow moving matrix around with mmb drag
Use document matrix to shift origin of svg (through either transform property in a group or viewbox property on svg)
Tools offset by document transform.

This change is

cloudflare-workers-and-pages · 2021-06-10T17:37:08Z

Deploying with Cloudflare Pages

Latest commit:	`63e009d`
Status:	✅ Deploy successful!
Preview URL:	https://ab494344.graphite-master.pages.dev

View logs

core/document/src/document.rs

…backend (#180) * Backend and Frontend modification to show working color mods * Remove comments & change precedence for tool and doc actions * Add keybind for resetting work colors * Minor Frontend changes

TrueDoctor

Reviewed 2 of 6 files at r1, 8 of 17 files at r3, 7 of 8 files at r6, 7 of 8 files at r7.
Reviewable status: 24 of 30 files reviewed, 5 unresolved discussions (waiting on @0HyperCube)

core/document/Cargo.toml, line 15 at r7 (raw file):

# TODO: Swich to kurbo release when derive `PartialEq` is available for BezPath
#kurbo = "0.8.0"
kurbo = {git="https://github.com/0HyperCube/kurbo.git", branch="bezpath-partial-eq"}

Could you switch to the graphite fork?

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

				.enumerate()
				.map(|(i, f)| f.to_string() + if i != 5 { "," } else { "" })
				.collect::<String>()

You should not need to collect to a string here if you move the write call into the lambda function

core/document/src/layers/line.rs, line 16 at r7 (raw file):

				(cols[4] + cols[0] * 0. + cols[1] * 0., cols[5] + cols[2] * 0. + cols[3] * 0.),
				(cols[4] + cols[0] * 1. + cols[1] * 1., cols[5] + cols[2] * 1. + cols[3] * 1.),

so you basically transform the unit line?

Could you use glam for the matrix multiplication?
Would it make sense to add a from_points() method?

core/editor/src/document/document_message_handler.rs, line 180 at r7 (raw file):

					let delta = DVec2::new(ipp.mouse.position.x as f64 - self.mouse_pos.x as f64, ipp.mouse.position.y as f64 - self.mouse_pos.y as f64);
					let transform = self.active_document().document.root.transform * DAffine2::from_translation(delta);
					self.active_document_mut().document.root.transform = transform;

I am not sure I like the direct modification of the document without using Operations but there is probably an argument to be made for your current approach.
We do eventually need an UpdateTransform operation.

core/editor/src/document/document_message_handler.rs, line 197 at r7 (raw file):

			actions!(DocumentMessageDiscriminant; Undo, DeleteSelectedLayers, RenderDocument, ExportDocument, MouseMove, TranslateUp, TranslateDown)
		} else {
			actions!(DocumentMessageDiscriminant; Undo, RenderDocument, ExportDocument, MouseMove, TranslateUp)

Why do you make this destinction?

0HyperCube

Reviewable status: 19 of 30 files reviewed, 5 unresolved discussions (waiting on @TrueDoctor)

core/document/Cargo.toml, line 15 at r7 (raw file):

Previously, TrueDoctor wrote…

Could you switch to the graphite fork?

Done.

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

Previously, TrueDoctor wrote…

You should not need to collect to a string here if you move the write call into the lambda function

Done.

core/document/src/layers/line.rs, line 16 at r7 (raw file):

Previously, TrueDoctor wrote…

				(cols[4] + cols[0] * 0. + cols[1] * 0., cols[5] + cols[2] * 0. + cols[3] * 0.),
				(cols[4] + cols[0] * 1. + cols[1] * 1., cols[5] + cols[2] * 1. + cols[3] * 1.),
so you basically transform the unit line?

Could you use glam for the matrix multiplication?

Would it make sense to add a from_points() method?

yes

Probably
I converted the operation to take in the points.
Should I do the same with the rect?

core/editor/src/document/document_message_handler.rs, line 180 at r7 (raw file):

Previously, TrueDoctor wrote…

I am not sure I like the direct modification of the document without using Operations but there is probably an argument to be made for your current approach.
We do eventually need an UpdateTransform operation.

Indeed. Should there be separate translate document / rotate document / scale document events?

core/editor/src/document/document_message_handler.rs, line 197 at r7 (raw file):

Previously, TrueDoctor wrote…

			actions!(DocumentMessageDiscriminant; Undo, DeleteSelectedLayers, RenderDocument, ExportDocument, MouseMove, TranslateUp, TranslateDown)
		} else {
			actions!(DocumentMessageDiscriminant; Undo, RenderDocument, ExportDocument, MouseMove, TranslateUp)

Why do you make this destinction?

This wasn't intentional. It is now fixed.

core/document/src/layers/mod.rs

core/document/src/layers/shape.rs

TrueDoctor

Reviewed 3 of 8 files at r8, 1 of 1 files at r10, 8 of 14 files at r11, 1 of 3 files at r12, 1 of 4 files at r14, 3 of 3 files at r15.
Reviewable status: 27 of 32 files reviewed, 9 unresolved discussions (waiting on @0HyperCube, @pkupper, and @TrueDoctor)

core/document/src/document.rs, line 258 at r15 (raw file):

				self.work_operations.clear();
				self.work_mount_path = vec![];
				self.work = Layer::new(LayerDataTypes::Folder(Folder::default()), [1., 0., 0., 1., 0., 0.], PathStyle::default());

you can use affine::default here if you want because it is document internal (but we can do that in the second pr(then we could use Affine::default()))

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

	fn to_kurbo_path(&mut self, _: glam::DAffine2, _: style::PathStyle) -> kurbo::BezPath {
		unimplemented!()

I feel like this in not ideal, we might be able to clean this up if we end up merging all shapes into point generators

core/document/src/layers/line.rs, line 24 at r15 (raw file):

		}
		let mut path = kurbo::BezPath::new();
		path.move_to(new_point(transform.transform_point2(glam::const_dvec2!([0., 0.]))));

isn't this just equivalent to the translation?

core/document/src/layers/line.rs, line 25 at r15 (raw file):

		let mut path = kurbo::BezPath::new();
		path.move_to(new_point(transform.transform_point2(glam::const_dvec2!([0., 0.]))));
		path.line_to(new_point(transform.transform_point2(glam::const_dvec2!([1., 1.]))));

you can also use (1.,1.) with the from impl or the glam one vec constant for this

core/document/src/layers/mod.rs, line 140 at r15 (raw file):

Quoted 4 lines of code…

	pub fn render_as_folder(&mut self, svg: &mut String) {
		match &mut self.data {
			LayerDataTypes::Folder(f) => f.render(svg, self.transform, self.style),
			_ => {}

This does seem a bit dirty but I can live with that for temp code

core/document/src/layers/rect.rs, line 24 at r15 (raw file):

		}
		let mut path = kurbo::BezPath::new();
		path.move_to(new_point(transform.transform_point2(glam::const_dvec2!([0., 0.]))));

you could think about using something like [(0,0), {0,1}, (1,0), (1,1)].map(transform.transform_point).foreach(|x| path.line_to(Point::new(x.x, x.y)))

0HyperCube

Reviewable status: 24 of 32 files reviewed, 9 unresolved discussions (waiting on @0HyperCube, @pkupper, and @TrueDoctor)

core/document/src/document.rs, line 258 at r15 (raw file):

Previously, TrueDoctor wrote…

you can use affine::default here if you want because it is document internal (but we can do that in the second pr(then we could use Affine::default()))

I changed it to DAffine2::IDENTITY.

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

Previously, TrueDoctor wrote…

I feel like this in not ideal, we might be able to clean this up if we end up merging all shapes into point generators

Yes, either the folder needs to be a special case of layer or to_kurbo_path could return a vec of BezPath.

core/document/src/layers/line.rs, line 24 at r15 (raw file):

Previously, TrueDoctor wrote…

isn't this just equivalent to the translation?

Done

core/document/src/layers/line.rs, line 25 at r15 (raw file):

Previously, TrueDoctor wrote…

you can also use (1.,1.) with the from impl or the glam one vec constant for this

Done.

core/document/src/layers/shape.rs, line 37 at r9 (raw file):

Previously, 0HyperCube wrote…

I was thinking that that should probably happen, I'll add a TODO.

Done.

TrueDoctor

Reviewed 1 of 14 files at r11, 2 of 3 files at r16, 5 of 5 files at r17.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @0HyperCube, @pkupper, and @TrueDoctor)

core/document/src/document.rs, line 258 at r15 (raw file):

Previously, 0HyperCube wrote…

I changed it to DAffine2::IDENTITY.

Yeah, I was thinking about changing the method argument to take a transform struct it is a bit cleaner if the implementation of the document lib does not have to deal with conversisons and we can instead bundle all type conversions in one place, but we can change that later

core/document/src/layers/rect.rs, line 27 at r17 (raw file):

		[(1., 0.), (1., 1.), (0., 1.)]
			.iter()
			.for_each(|(x, y)| path.line_to(new_point(transform.transform_point2(DVec2::new(*x, *y)))));

Minor nitpick, glam supports From<(f32, f32)> for vec. so you should be able to get rid of the DVec2 constructor if you use the new shiny into_iterator() intsead of iter() (to get rid of the ref)

0HyperCube

Dismissed @TrueDoctor and @pkupper from 4 discussions.
Reviewable status: 31 of 32 files reviewed, 2 unresolved discussions (waiting on @0HyperCube and @TrueDoctor)

core/document/src/layers/rect.rs, line 24 at r15 (raw file):

Previously, TrueDoctor wrote…

you could think about using something like [(0,0), {0,1}, (1,0), (1,1)].map(transform.transform_point).foreach(|x| path.line_to(Point::new(x.x, x.y)))

Done.

TrueDoctor

Reviewable status: 31 of 32 files reviewed, 2 unresolved discussions (waiting on @0HyperCube and @TrueDoctor)

0HyperCube

Dismissed @TrueDoctor from a discussion.
Reviewable status: 31 of 32 files reviewed, all discussions resolved (waiting on @TrueDoctor)

core/document/src/layers/mod.rs, line 140 at r15 (raw file):

Previously, TrueDoctor wrote…

	pub fn render_as_folder(&mut self, svg: &mut String) {
		match &mut self.data {
			LayerDataTypes::Folder(f) => f.render(svg, self.transform, self.style),
			_ => {}

This does seem a bit dirty but I can live with that for temp code

Ok. I'm unsure how this should be improved

core/document/src/layers/rect.rs, line 27 at r17 (raw file):

Previously, TrueDoctor wrote…

Minor nitpick, glam supports From<(f32, f32)> for vec. so you should be able to get rid of the DVec2 constructor if you use the new shiny into_iterator() intsead of iter() (to get rid of the ref)

Ok. into_iter() gave me the warning below but have changed it to use .into instead of the constructor.

warning: this method call currently resolves to `<&[T; N] as IntoIterator>::into_iter` (due to autoref coercions), but that might change in the future when `IntoIterator` impls for arrays are added.
  --> core/document/src/layers/rect.rs:25:34
   |
25 |         [(1., 0.), (1., 1.), (0., 1.)].into_iter().for_each(|v| path.line_to(new_point(transform.transform_point2((*v).into()))));
   |                                        ^^^^^^^^^ help: use `.iter()` instead of `.into_iter()` to avoid ambiguity: `iter`
   |
   = note: `#[warn(array_into_iter)]` on by default
   = warning: this changes meaning in Rust 2021
   = note: for more information, see issue #66145 <https://github.com/rust-lang/rust/issues/66145>

TrueDoctor

Reviewed 1 of 1 files at r18, 7 of 7 files at r19.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @0HyperCube)

core/document/src/layers/rect.rs, line 27 at r17 (raw file):

Previously, 0HyperCube wrote…

Ok. into_iter() gave me the warning below but have changed it to use .into instead of the constructor.

warning: this method call currently resolves to `<&[T; N] as IntoIterator>::into_iter` (due to autoref coercions), but that might change in the future when `IntoIterator` impls for arrays are added.
  --> core/document/src/layers/rect.rs:25:34
   |
25 |         [(1., 0.), (1., 1.), (0., 1.)].into_iter().for_each(|v| path.line_to(new_point(transform.transform_point2((*v).into()))));
   |                                        ^^^^^^^^^ help: use `.iter()` instead of `.into_iter()` to avoid ambiguity: `iter`
   |
   = note: `#[warn(array_into_iter)]` on by default
   = warning: this changes meaning in Rust 2021
   = note: for more information, see issue #66145 <https://github.com/rust-lang/rust/issues/66145>

This seems to be due to backwards compatibility: https://blog.rust-lang.org/2021/06/17/Rust-1.53.0.html
The behaviour will change with rust 2021. but because the tuples are copy anyway IntoIterator::into_iter( would just be unnecessarily verbose. Just so you know, with bigger elements into_iter should be more efficient

0HyperCube

Reviewable status: 31 of 32 files reviewed, all discussions resolved (waiting on @TrueDoctor)

core/document/src/layers/rect.rs, line 27 at r17 (raw file):

Previously, TrueDoctor wrote…

This seems to be due to backwards compatibility: https://blog.rust-lang.org/2021/06/17/Rust-1.53.0.html
The behaviour will change with rust 2021. but because the tuples are copy anyway IntoIterator::into_iter( would just be unnecessarily verbose. Just so you know, with bigger elements into_iter should be more efficient

Ok I've added a TODO.

TrueDoctor

Reviewable status: 31 of 32 files reviewed, 1 unresolved discussion (waiting on @0HyperCube and @TrueDoctor)

core/document/src/document.rs, line 48 at r20 (raw file):

			// TODO: Handle if mounted in nested folders
			let transform = self.document_folder(path).unwrap().transform.clone();
			self.document_folder_mut(path).unwrap().render_as_folder(svg);

minor nitpick afaik DAffine2 is copy, hence no need to clone

core/document/src/layers/mod.rs, line 140 at r15 (raw file):

Previously, 0HyperCube wrote…

Ok. I'm unsure how this should be improved

afai can tell, the only difference to the regular render function is, that this one ignores caches and visibility meta information, is there a reason to not just use the regular render?Fals

0HyperCube

Reviewable status: 30 of 32 files reviewed, all discussions resolved (waiting on @TrueDoctor)

core/document/src/document.rs, line 48 at r20 (raw file):

Previously, TrueDoctor wrote…

minor nitpick afaik DAffine2 is copy, hence no need to clone

Done.

core/document/src/layers/mod.rs, line 140 at r15 (raw file):

Previously, TrueDoctor wrote…

afai can tell, the only difference to the regular render function is, that this one ignores caches and visibility meta information, is there a reason to not just use the regular render?Fals

Currently, if I add a layer to the root, the root (and any other parent folders) will not be flagged as dirty and so will use the cached svg without the new layer added.

TrueDoctor

Reviewed 1 of 1 files at r20, 1 of 1 files at r21.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @0HyperCube)

* Convert polygon and rectangle tool to kurbo::BezPath * Add glam * Add affine transform to elipse and remove circle * Format * Add svg group and add matrix for group * Convert all operations to use matricies * Work uses same transform as root * Format * Frontend fixed to render changes to working colors when changed from backend (#180) * Backend and Frontend modification to show working color mods * Remove comments & change precedence for tool and doc actions * Add keybind for resetting work colors * Minor Frontend changes * Remove early sample "greet" code * Add a contributing section to the project README * Add moving document around * Add document transform for tools * Update to GraphiteEditor's fork * Use write in foreach for rendering group / folder * Add missing TranslateDown action * Use points for line operation * Format * Add todo to change to shape's aspect ratio * Remove empty if * Initial pass at refactor * Fix polyline test * Use document message to modify document transform * Messages -> Operations * Transform layer * Format * Use DAffine2::IDENTITY * Clean up kurbo generation for line and rect * Use .into for rectangle points * Rename cols to transform * Rename other cols to transform * Add todo for into_iter * Remove unnecessary clone Co-authored-by: akshay1992kalbhor <[email protected]> Co-authored-by: Keavon Chambers <[email protected]>

0HyperCube and others added 2 commits June 10, 2021 17:41

Convert polygon and rectangle tool to kurbo::BezPath

4d638c6

Merge branch 'master' into matrix-transforms

4ce2f7e

Add glam

8450cdd

TrueDoctor linked an issue Jun 10, 2021 that may be closed by this pull request

Choice of a linear algebra library #165

Closed

0HyperCube added 3 commits June 10, 2021 21:46

Add affine transform to elipse and remove circle

e8145f3

Format

436fd2a

Add svg group and add matrix for group

fa8189e

Keavon linked an issue Jun 13, 2021 that may be closed by this pull request

Document scaling #100

Closed

0HyperCube added 3 commits June 13, 2021 08:46

Convert all operations to use matricies

3b9dd7b

Work uses same transform as root

47ee67f

Format

854715d

TrueDoctor reviewed Jun 13, 2021

View reviewed changes

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

akshay1992kalbhor and others added 5 commits June 13, 2021 09:16

Frontend fixed to render changes to working colors when changed from …

b737e27

…backend (#180) * Backend and Frontend modification to show working color mods * Remove comments & change precedence for tool and doc actions * Add keybind for resetting work colors * Minor Frontend changes

Remove early sample "greet" code

df1a95e

Add a contributing section to the project README

70c7917

Merge branch 'master' into matrix-transforms

00aa790

Add moving document around

ba1d4f4

0HyperCube force-pushed the matrix-transforms branch from b5048b8 to ba1d4f4 Compare June 13, 2021 09:42

0HyperCube and others added 2 commits June 13, 2021 13:53

Add document transform for tools

6b664f9

Merge branch 'master' into matrix-transforms

89b6263

0HyperCube marked this pull request as ready for review June 13, 2021 13:08

TrueDoctor requested changes Jun 13, 2021

View reviewed changes

0HyperCube added 4 commits June 13, 2021 15:13

Update to GraphiteEditor's fork

d10f74a

Use write in foreach for rendering group / folder

8991094

Add missing TranslateDown action

ecd3f34

Use points for line operation

dd8b048

0HyperCube commented Jun 13, 2021

View reviewed changes

Format

ce66e35

pkupper reviewed Jun 13, 2021

View reviewed changes

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

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

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

0HyperCube and others added 6 commits June 24, 2021 12:37

Merge branch 'master' into matrix-transforms

0cfa296

Merge branch 'master' into matrix-transforms

f4ef8fc

Use document message to modify document transform

b015239

Messages -> Operations

b56a64a

Transform layer

a1bbc9c

Format

b715fd4

TrueDoctor requested changes Jun 25, 2021

View reviewed changes

0HyperCube added 2 commits June 25, 2021 22:22

Use DAffine2::IDENTITY

25e5aae

Clean up kurbo generation for line and rect

6be462f

0HyperCube commented Jun 25, 2021

View reviewed changes

TrueDoctor reviewed Jun 26, 2021

View reviewed changes

Use .into for rectangle points

19a164d

0HyperCube commented Jun 26, 2021

View reviewed changes

TrueDoctor approved these changes Jun 26, 2021

View reviewed changes

0HyperCube commented Jun 26, 2021

View reviewed changes

pkupper approved these changes Jun 26, 2021

View reviewed changes

0HyperCube added 2 commits June 26, 2021 20:02

Rename cols to transform

0b10a60

Rename other cols to transform

663415b

TrueDoctor approved these changes Jun 26, 2021

View reviewed changes

Add todo for into_iter

db6cc50

0HyperCube commented Jun 26, 2021

View reviewed changes

TrueDoctor reviewed Jun 26, 2021

View reviewed changes

Remove unnecessary clone

63e009d

0HyperCube commented Jun 26, 2021

View reviewed changes

TrueDoctor approved these changes Jun 26, 2021

View reviewed changes

0HyperCube merged commit f76517a into master Jun 26, 2021

0HyperCube deleted the matrix-transforms branch June 26, 2021 20:44

Keavon mentioned this pull request Jul 3, 2021

Select Tool: Viewport layer click selection #140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a movable canvas with matricies #175

Add a movable canvas with matricies #175

0HyperCube commented Jun 10, 2021 •

edited

Loading

cloudflare-workers-and-pages bot commented Jun 10, 2021 •

edited

Loading

TrueDoctor left a comment

0HyperCube left a comment

TrueDoctor left a comment

0HyperCube left a comment

TrueDoctor left a comment

0HyperCube left a comment

TrueDoctor left a comment

0HyperCube left a comment

TrueDoctor left a comment

0HyperCube left a comment

TrueDoctor left a comment

0HyperCube left a comment

TrueDoctor left a comment

Add a movable canvas with matricies #175

Add a movable canvas with matricies #175

Conversation

0HyperCube commented Jun 10, 2021 • edited Loading

cloudflare-workers-and-pages bot commented Jun 10, 2021 • edited Loading

Deploying with Cloudflare Pages

TrueDoctor left a comment

Choose a reason for hiding this comment

0HyperCube left a comment

Choose a reason for hiding this comment

TrueDoctor left a comment

Choose a reason for hiding this comment

0HyperCube left a comment

Choose a reason for hiding this comment

TrueDoctor left a comment

Choose a reason for hiding this comment

0HyperCube left a comment

Choose a reason for hiding this comment

TrueDoctor left a comment

Choose a reason for hiding this comment

0HyperCube left a comment

Choose a reason for hiding this comment

TrueDoctor left a comment

Choose a reason for hiding this comment

0HyperCube left a comment

Choose a reason for hiding this comment

TrueDoctor left a comment

Choose a reason for hiding this comment

0HyperCube left a comment

Choose a reason for hiding this comment

TrueDoctor left a comment

Choose a reason for hiding this comment

0HyperCube commented Jun 10, 2021 •

edited

Loading

cloudflare-workers-and-pages bot commented Jun 10, 2021 •

edited

Loading