WIP: Eraser implementation #7

Draft
Franciman wants to merge 8 commits from eraserhead into master
Collaborator

On this branch there is a naive implementation of an eraser for erasing parts of the stroke.

I think that at this stage the important thing to get right is the architecture of the various tools that can be used to interact with the canvas: I tried and made a small first step in this direction, but the result seems unsatisfactory.

Important thing to do: Use a better User Interface that makes it clear which drawing tool is active

On this branch there is a naive implementation of an eraser for erasing parts of the stroke. I think that at this stage the important thing to get right is the architecture of the various tools that can be used to interact with the canvas: I tried and made a small first step in this direction, but the result seems unsatisfactory. Important thing to do: Use a better User Interface that makes it clear which drawing tool is active
Franciman added this to the 0.1 milestone 2020-11-09 16:33:49 +01:00
enrico reviewed 2020-11-09 17:19:00 +01:00
enrico left a comment
Owner

This is clearly an important architectural piece, some work is needed, but we're heading in the right direction

This is clearly an important architectural piece, some work is needed, but we're heading in the right direction
src/lib.rs Outdated
@ -40,0 +51,4 @@
|| rect.contains(cubic.p1)
|| rect.contains(cubic.p2),
}
}
Owner

I'm not sure about this code: if a line intersects a Rect, but the line start and end are outside of the rect, will this return true?

I'm not sure about this code: if a line intersects a Rect, but the line start and end are outside of the rect, will this return true?
src/lib.rs Outdated
@ -43,0 +74,4 @@
CanvasElement::Freehand{path, thickness: _} => {
// Remove segments intersecting the eraser
let new_segments = (*path).kurbo_path.segments().filter(|path| ! segment_intersects_rect(path, eraser_rect));
path.kurbo_path = druid::kurbo::BezPath::from_path_segments(new_segments);
Owner

I think we should split the various parts in more Canvas::Freehand parts. This way, when we implement selection etc, we can identify paths with a single connected stroke

I think we should split the various parts in more Canvas::Freehand parts. This way, when we implement selection etc, we can identify paths with a single connected stroke
src/lib.rs Outdated
@ -43,0 +81,4 @@
}
}
pub fn push_back(&mut self, element: CanvasElement) {
Owner

Maybe "add_element"?

Maybe "add_element"?
src/main.rs Outdated
@ -24,0 +24,4 @@
// Tools that can be used to interact with the canvas
#[derive(Clone, Data)]
enum CanvasTool {
Pen { current_element: Option<CanvasElement> },
Owner

I'm not sure that current_element should be associated with the Pen; a Pen should have thickness and color informations only

I'm not sure that current_element should be associated with the Pen; a Pen should have thickness and color informations only
Author
Collaborator

When does the current path being drawn make sense?
I felt a little uncomfortable keeping this in CanvasData, because there is no current path being drawn when we erase. Probably with the state machine we can achieve a cleaner vision, though.

When does `the current path being drawn` make sense? I felt a little uncomfortable keeping this in CanvasData, because there is no current path being drawn when we erase. Probably with the state machine we can achieve a cleaner vision, though.
Owner

It still fits the CanvasData, because it is a piece of data associated to the Canvas (indeed, the currently drawn item). When there is no currently drawn item, is set to None; for the time being, current_element != None is the result of is_drawing()

It still fits the CanvasData, because it is a piece of data associated to the Canvas (indeed, the currently drawn item). When there is no currently drawn item, is set to None; for the time being, current_element != None is the result of is_drawing()
@ -25,3 +41,2 @@
struct CanvasData {
current_element: Option<CanvasElement>,
//elements: Vector<CanvasElement>,
current_tool: CanvasTool,
Owner

Nice

Nice
enrico marked this conversation as resolved
src/main.rs Outdated
@ -113,0 +183,4 @@
}
_ => {
// we just changed the canvas tool, there is no need to repaint
return;
Owner

I feel like we should use a state machine to identify the state the app or the canvas is in; maybe this would simplify this logic, or at least make it more explicit

I feel like we should use a state machine to identify the state the app or the canvas is in; maybe this would simplify this logic, or at least make it more explicit
src/main.rs Outdated
@ -144,3 +216,3 @@
ctx.fill(rect, &Color::WHITE);
for element in data.elements.get().iter() {
for element in data.elements.get().elements.iter() {
Owner

lol the name choices are unfortunate

lol the name choices are unfortunate
src/main.rs Outdated
Owner

I think this is fine for now

I think this is fine for now
Owner

Small brain dump:

  1. we need to put the AGPL header on every new file
  2. When a stroke is fully cancelled, it shall be removed from the canvas vector
Small brain dump: 1) we need to put the AGPL header on every new file 2) When a stroke is fully cancelled, it shall be removed from the canvas vector
Author
Collaborator

2 has been taken care of

2 has been taken care of
Author
Collaborator

In the algorithms that implement strokes erasing, I create a new im::Vector each time, so it's a little bit expensive, it should be fixed.

Also, we must take care of the matter of finding a way to determine (cheaply) whether two Canvas versions are actually different or not: For now I'm using a version_id that gets incremented each time an actual edit (i.e. moving the eraser without touching any stroke is not an actual edit) is performed.

In the algorithms that implement strokes erasing, I create a new im::Vector each time, so it's a little bit expensive, it should be fixed. Also, we must take care of the matter of finding a way to determine (cheaply) whether two Canvas versions are actually different or not: For now I'm using a version_id that gets incremented each time an actual edit (i.e. moving the eraser without touching any stroke is not an actual edit) is performed.
Owner

After thinking a bit more about the statefulness of tools, I think we should implement the two following tools as a test:

  1. pen that changes color while drawing
  2. tool to create a closed polygon by pressing consecutively on points in the canvas
After thinking a bit more about the statefulness of tools, I think we should implement the two following tools as a test: 1) pen that changes color while drawing 2) tool to create a closed polygon by pressing consecutively on points in the canvas
Owner

After more experimentation, I think we should give even more effort to this feature.

In the final application the user would have a palette of costumizable tools, e.g.: one black pen, one red pen, etc. So some tools would be fixed (like the eraser), while others would be dynamically created.

This means that our Widget data contains something along the line of Vec<CanvasTool>.

After more experimentation, I think we should give even more effort to this feature. In the final application the user would have a palette of costumizable tools, e.g.: one black pen, one red pen, etc. So some tools would be fixed (like the eraser), while others would be dynamically created. This means that our Widget data contains something along the line of `Vec<CanvasTool>`.
enrico modified the milestone from 0.1 to Backlog 2021-03-01 13:47:19 +01:00
enrico added the
drawing
enhancement
labels 2021-03-03 12:18:38 +01:00
enrico modified the milestone from Backlog to 1.0 2021-03-03 12:19:08 +01:00
This pull request has changes conflicting with the target branch.
  • src/canvas.rs
  • src/lib.rs
  • src/main.rs
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b eraserhead master
git pull origin eraserhead

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff eraserhead
git push origin master
Sign in to join this conversation.
No description provided.