Skip to content

Refactor internal shape and remove reliance on Kurbo (PR #617) - Disentangle Kurbo#619

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

Merged
caleb-ad merged 4 commits into from
Apr 28, 2022
Merged

Refactor internal shape and remove reliance on Kurbo (PR #617) - Disentangle Kurbo #619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6155c04
Refactoring: disentangle kurbo from apply_affine
caleb-ad Apr 25, 2022
5743448
Broke boolean operations, refactor in state which compiles
caleb-ad Apr 26, 2022
7cf07cd
"fixed" boolean operation refactor related errors
caleb-ad Apr 26, 2022
15ebe95
fixed apply_affine, which would not have applied any type of affine
caleb-ad Apr 26, 2022
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
2 changes: 1 addition & 1 deletion editor/src/document/document_message_handler.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -12,13 +12,13 @@ use crate::layout::widgets::{
WidgetCallback, WidgetHolder, WidgetLayout,
};
use crate::message_prelude::*;
use crate::viewport_tools::vector_editor::vector_shape::VectorShape;
use crate::EditorError;

use graphene::document::Document as GrapheneDocument;
use graphene::layers::folder_layer::FolderLayer;
use graphene::layers::layer_info::LayerDataType;
use graphene::layers::style::ViewMode;
use graphene::layers::vector::vector_shape::VectorShape;
use graphene::{DocumentError, DocumentResponse, LayerId, Operation as DocumentOperation};

use glam::{DAffine2, DVec2};
Expand Down
2 changes: 1 addition & 1 deletion editor/src/viewport_tools/tools/pen_tool.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -8,9 +8,9 @@ use crate::misc::{HintData, HintGroup, HintInfo, KeysGroup};
use crate::viewport_tools::snapping::SnapHandler;
use crate::viewport_tools::tool::{DocumentToolData, Fsm, ToolActionHandlerData};
use crate::viewport_tools::vector_editor::shape_editor::ShapeEditor;
use crate::viewport_tools::vector_editor::vector_shape::VectorShape;

use graphene::layers::style;
use graphene::layers::vector::vector_shape::VectorShape;
use graphene::Operation;

use glam::{DAffine2, DVec2};
Expand Down
7 changes: 3 additions & 4 deletions editor/src/viewport_tools/vector_editor/mod.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
pub mod constants;
pub mod overlay_renderer;
pub mod shape_adapter;
pub mod shape_editor;
pub mod vector_anchor;
pub mod vector_control_point;
pub mod vector_shape;
use graphene::layers::vector::vector_anchor;
use graphene::layers::vector::vector_control_point;
use graphene::layers::vector::vector_shape;
34 changes: 0 additions & 34 deletions editor/src/viewport_tools/vector_editor/shape_adapter.rs
View file
Open in desktop

This file was deleted.

60 changes: 32 additions & 28 deletions graphene/src/boolean_ops.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3,7 +3,7 @@ use crate::intersection::{intersections, line_curve_intersections, valid_t, Inte
use crate::layers::shape_layer::ShapeLayer;
use crate::layers::style::PathStyle;

use kurbo::{BezPath, CubicBez, Line, ParamCurve, ParamCurveArclen, ParamCurveArea, ParamCurveExtrema, PathEl, PathSeg, Point, QuadBez, Rect};
use kurbo::{BezPath, CubicBez, Line, ParamCurve, ParamCurveArclen, ParamCurveArea, ParamCurveExtrema, PathEl, PathSeg, Point, QuadBez, Rect, Shape};
use serde::{Deserialize, Serialize};
use std::fmt::{self, Debug, Formatter};

Expand DownExpand Up@@ -453,23 +453,23 @@ pub fn subdivide_path_seg(p: &PathSeg, t_values: &mut [f64]) -> Vec<Option<PathS
sub_segments
}

// TODO: check if shapes are filled
// TODO: Bug: shape with at least two subpaths and comprised of many unions sometimes has erroneous movetos embedded in edges
// TODO: reduce copying
pub fn boolean_operation(select: BooleanOperation, mut alpha: ShapeLayer, mut beta: ShapeLayer) -> Result<Vec<ShapeLayer>, BooleanOperationError> {
if alpha.shape.is_empty() || beta.shape.is_empty() {
if alpha.shape.anchors.len() < 2 || beta.shape.anchors.len() < 2 {
return Err(BooleanOperationError::InvalidSelection);
}
alpha.shape = close_path(&alpha.shape);
beta.shape = close_path(&beta.shape);
let beta_reverse = close_path(&reverse_path(&beta.shape));
let alpha_dir = Cycle::direction_for_path(&alpha.shape)?;
let beta_dir = Cycle::direction_for_path(&beta.shape)?;
let mut alpha_shape = close_path(&(&alpha.shape).into());
let mut beta_shape = close_path(&(&beta.shape).into());
let beta_reverse = close_path(&reverse_path(&beta_shape));
let alpha_dir = Cycle::direction_for_path(&alpha_shape)?;
let beta_dir = Cycle::direction_for_path(&beta_shape)?;
match select {
BooleanOperation::Union => {
match if beta_dir == alpha_dir {
PathGraph::from_paths(&alpha.shape, &beta.shape)
PathGraph::from_paths(&alpha_shape, &beta_shape)
} else {
PathGraph::from_paths(&alpha.shape, &beta_reverse)
PathGraph::from_paths(&alpha_shape, &beta_reverse)
} {
Ok(graph) => {
let mut cycles = graph.get_cycles();
Expand All@@ -479,16 +479,20 @@ pub fn boolean_operation(select: BooleanOperation, mut alpha: ShapeLayer, mut be
&alpha.style,
);
for interior in collect_shapes(&graph, &mut cycles, |dir| dir != alpha_dir, |_| &alpha.style)? {
add_subpath(&mut boolean_union.shape, interior.shape);
//TODO: this is not very efficient or nice to read
let mut a_path: BezPath = (&boolean_union.shape).into();
let b_path: BezPath = (&interior.shape).into();
add_subpath(&mut a_path, b_path);
boolean_union.shape = a_path.iter().into();
}
Ok(vec![boolean_union])
}
Err(BooleanOperationError::NoIntersections) => {
// If shape is inside the other the Union is just the larger
// Check could also be done with area and single ray cast
if cast_horizontal_ray(point_on_curve(&beta.shape), &alpha.shape) % 2 != 0 {
if cast_horizontal_ray(point_on_curve(&beta_shape), &alpha_shape) % 2 != 0 {
Ok(vec![alpha])
} else if cast_horizontal_ray(point_on_curve(&alpha.shape), &beta.shape) % 2 != 0 {
} else if cast_horizontal_ray(point_on_curve(&alpha_shape), &beta_shape) % 2 != 0 {
beta.style = alpha.style;
Ok(vec![beta])
} else {
Expand All@@ -500,17 +504,17 @@ pub fn boolean_operation(select: BooleanOperation, mut alpha: ShapeLayer, mut be
}
BooleanOperation::Difference => {
let graph = if beta_dir != alpha_dir {
PathGraph::from_paths(&alpha.shape, &beta.shape)?
PathGraph::from_paths(&alpha_shape, &beta_shape)?
} else {
PathGraph::from_paths(&alpha.shape, &beta_reverse)?
PathGraph::from_paths(&alpha_shape, &beta_reverse)?
};
collect_shapes(&graph, &mut graph.get_cycles(), |_| true, |dir| if dir == alpha_dir { &alpha.style } else { &beta.style })
}
BooleanOperation::Intersection => {
match if beta_dir == alpha_dir {
PathGraph::from_paths(&alpha.shape, &beta.shape)
PathGraph::from_paths(&alpha_shape, &beta_shape)
} else {
PathGraph::from_paths(&alpha.shape, &beta_reverse)
PathGraph::from_paths(&alpha_shape, &beta_reverse)
} {
Ok(graph) => {
let mut cycles = graph.get_cycles();
Expand All@@ -527,10 +531,10 @@ pub fn boolean_operation(select: BooleanOperation, mut alpha: ShapeLayer, mut be
}
Err(BooleanOperationError::NoIntersections) => {
// Check could also be done with area and single ray cast
if cast_horizontal_ray(point_on_curve(&beta.shape), &alpha.shape) % 2 != 0 {
if cast_horizontal_ray(point_on_curve(&beta_shape), &alpha_shape) % 2 != 0 {
beta.style = alpha.style;
Ok(vec![beta])
} else if cast_horizontal_ray(point_on_curve(&alpha.shape), &beta.shape) % 2 != 0 {
} else if cast_horizontal_ray(point_on_curve(&alpha_shape), &beta_shape) % 2 != 0 {
Ok(vec![alpha])
} else {
Err(BooleanOperationError::NothingDone)
Expand All@@ -541,14 +545,14 @@ pub fn boolean_operation(select: BooleanOperation, mut alpha: ShapeLayer, mut be
}
BooleanOperation::SubtractBack => {
match if beta_dir != alpha_dir {
PathGraph::from_paths(&alpha.shape, &beta.shape)
PathGraph::from_paths(&alpha_shape, &beta_shape)
} else {
PathGraph::from_paths(&alpha.shape, &beta_reverse)
PathGraph::from_paths(&alpha_shape, &beta_reverse)
} {
Ok(graph) => collect_shapes(&graph, &mut graph.get_cycles(), |dir| dir != alpha_dir, |_| &beta.style),
Err(BooleanOperationError::NoIntersections) => {
if cast_horizontal_ray(point_on_curve(&alpha.shape), &beta.shape) % 2 != 0 {
add_subpath(&mut beta.shape, if beta_dir == alpha_dir { reverse_path(&alpha.shape) } else { alpha.shape });
if cast_horizontal_ray(point_on_curve(&alpha_shape), &beta_shape) % 2 != 0 {
add_subpath(&mut beta_shape, if beta_dir == alpha_dir { reverse_path(&alpha_shape) } else { alpha_shape });
beta.style = alpha.style;
Ok(vec![beta])
} else {
Expand All@@ -560,14 +564,14 @@ pub fn boolean_operation(select: BooleanOperation, mut alpha: ShapeLayer, mut be
}
BooleanOperation::SubtractFront => {
match if beta_dir != alpha_dir {
PathGraph::from_paths(&alpha.shape, &beta.shape)
PathGraph::from_paths(&alpha_shape, &beta_shape)
} else {
PathGraph::from_paths(&alpha.shape, &beta_reverse)
PathGraph::from_paths(&alpha_shape, &beta_reverse)
} {
Ok(graph) => collect_shapes(&graph, &mut graph.get_cycles(), |dir| dir == alpha_dir, |_| &alpha.style),
Err(BooleanOperationError::NoIntersections) => {
if cast_horizontal_ray(point_on_curve(&beta.shape), &alpha.shape) % 2 != 0 {
add_subpath(&mut alpha.shape, if beta_dir == alpha_dir { reverse_path(&beta.shape) } else { beta.shape });
if cast_horizontal_ray(point_on_curve(&beta_shape), &alpha_shape) % 2 != 0 {
add_subpath(&mut alpha_shape, if beta_dir == alpha_dir { reverse_path(&beta_shape) } else { beta_shape });
Ok(vec![alpha])
} else {
Err(BooleanOperationError::NothingDone)
Expand All@@ -594,7 +598,7 @@ pub fn cast_horizontal_ray(mut from: Point, into: &BezPath) -> usize {
};
let mut intersects = Vec::new();
for ref seg in into.segments() {
if seg.bounding_box().x1 > from.x {
if kurbo::ParamCurveExtrema::bounding_box(seg).x1 > from.x {
line_curve_intersections(&ray, seg, true, |_, b| valid_t(b), &mut intersects);
}
}
Expand Down
6 changes: 3 additions & 3 deletions graphene/src/document.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -106,7 +106,7 @@ impl Document {
match (self.multiply_transforms(path), &self.layer(path)?.data) {
(Ok(shape_transform), LayerDataType::Shape(shape)) => {
let mut new_shape = shape.clone();
new_shape.shape.apply_affine(Affine::new((undo_viewport * shape_transform).to_cols_array()));
new_shape.shape.apply_affine(undo_viewport * shape_transform);

@otdaviesotdaviesApr 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will this have any downstream consequences? Do we no longer need it to be affine or is it already affine? Just curious

@caleb-adcaleb-adApr 28, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

undo_viewport * shape_transform is glam::DAffine2, that line was just coverting to kurbo::Affine

@caleb-adcaleb-adApr 28, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

which now happens in the apply_affine function, but we should be able to implement that function without kurbo once we get affine transformations for the anchor points working

@caleb-adcaleb-adApr 28, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Kind of off topic, why does VectorAnchor::move_selected_points(&mut self, translation:DVec2, relative: bool, transform: &DAffine2) take both a translation and a transform?

@otdaviesotdaviesApr 28, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Originally we needed it to do some kind of kurbo space transform to viewport transform space I believe. We may need it again, we'll see!

shapes.push(new_shape);
}
(Ok(_), _) => return Err(DocumentError::InvalidPath),
Expand DownExpand Up@@ -694,7 +694,7 @@ impl Document {
self.mark_as_dirty(path)?;

if let LayerDataType::Shape(shape) = &mut self.layer_mut(path)?.data {
shape.shape = bez_path.clone();
shape.shape = bez_path.clone().iter().into();
}
Some(vec![DocumentChanged, LayerChanged { path: path.clone() }])
}
Expand All@@ -709,7 +709,7 @@ impl Document {
}

if let LayerDataType::Shape(shape) = &mut self.layer_mut(path)?.data {
shape.shape = bez_path.clone();
shape.shape = bez_path.clone().iter().into();
}
Some([vec![DocumentChanged, LayerChanged { path: path.clone() }], update_thumbnails_upstream(path)].concat())
}
Expand Down
20 changes: 10 additions & 10 deletions graphene/src/layers/shape_layer.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -30,7 +30,7 @@ impl LayerData for ShapeLayer {
let _ = write!(svg, "<!-- SVG shape has an invalid transform -->");
return;
}
path.apply_affine(glam_to_kurbo(transform));
path.apply_affine(transform);

let _ = writeln!(svg, r#"<g transform="matrix("#);
inverse.to_cols_array().iter().enumerate().for_each(|(i, entry)| {
Expand All@@ -48,14 +48,14 @@ impl LayerData for ShapeLayer {
if transform.matrix2 == DMat2::ZERO {
return None;
}
path.apply_affine(glam_to_kurbo(transform));
path.apply_affine(transform);

let kurbo::Rect { x0, y0, x1, y1 } = path.bounding_box();
Some([(x0, y0).into(), (x1, y1).into()])
}

fn intersects_quad(&self, quad: Quad, path: &mut Vec<LayerId>, intersections: &mut Vec<Vec<LayerId>>) {
if intersect_quad_bez_path(quad, &self.shape, self.style.fill().is_some()) {
if intersect_quad_bez_path(quad, &(&self.shape).into(), self.style.fill().is_some()) {
intersections.push(path.clone());
}
}
Expand All@@ -73,7 +73,7 @@ impl ShapeLayer {

pub fn from_bez_path(bez_path: BezPath, style: PathStyle, closed: bool) -> Self {
Self {
shape: bez_path,
shape: bez_path.iter().into(),
style,
render_index: 1,
closed,
Expand DownExpand Up@@ -106,7 +106,7 @@ impl ShapeLayer {
path.close_path();

Self {
shape: path,
shape: path.iter().into(),
style,
render_index: 1,
closed: true,
Expand All@@ -115,7 +115,7 @@ impl ShapeLayer {

pub fn rectangle(style: PathStyle) -> Self {
Self {
shape: kurbo::Rect::new(0., 0., 1., 1.).to_path(0.01),
shape: VectorShape::from_kurbo_shape(&kurbo::Rect::new(0., 0., 1., 1.).to_path(0.01)),

@otdaviesotdaviesApr 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a good stand in for now, but we will eventually want to define ways of creating shapes directly as VectorShapes. (Not a complaint, just context)

style,
render_index: 1,
closed: true,
Expand All@@ -124,7 +124,7 @@ impl ShapeLayer {

pub fn ellipse(style: PathStyle) -> Self {
Self {
shape: kurbo::Ellipse::from_rect(kurbo::Rect::new(0., 0., 1., 1.)).to_path(0.01),
shape: VectorShape::from_kurbo_shape(&kurbo::Ellipse::from_rect(kurbo::Rect::new(0., 0., 1., 1.)).to_path(0.01)),
style,
render_index: 1,
closed: true,
Expand All@@ -133,7 +133,7 @@ impl ShapeLayer {

pub fn line(style: PathStyle) -> Self {
Self {
shape: kurbo::Line::new((0., 0.), (1., 0.)).to_path(0.01),
shape: VectorShape::from_kurbo_shape(&kurbo::Line::new((0., 0.), (1., 0.)).to_path(0.01)),
style,
render_index: 1,
closed: false,
Expand All@@ -150,7 +150,7 @@ impl ShapeLayer {
.for_each(|(i, p)| if i == 0 { path.move_to(p) } else { path.line_to(p) });

Self {
shape: path,
shape: path.iter().into(),
style,
render_index: 0,
closed: false,
Expand DownExpand Up@@ -228,7 +228,7 @@ impl ShapeLayer {
}

Self {
shape: path,
shape: path.iter().into(),
style,
render_index: 0,
closed: false,
Expand Down
Loading