Skip to content
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.

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

Add free draw mode #1570

Merged
merged 14 commits into from May 12, 2020
Merged

Add free draw mode #1570

merged 14 commits into from May 12, 2020

Conversation

kbariotis
Copy link
Contributor

@kbariotis kbariotis commented May 10, 2020

This replicates the line/arrow mode, but adds a point to the line on each mouse moving position.

For #25 and #1569

@vercel
Copy link

vercel bot commented May 10, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/d8mkv4xu6
✅ Preview: https://excalidraw-git-add-free-draw.excalidraw.now.sh

@dwelle
Copy link
Member

dwelle commented May 10, 2020

Nice.

If we end up using the same API as we do for lines, we really need to work on fixing the temporal consistency issues (#760), though it creates some psychedelic visuals:

excal_temporal_inconsistency

We'll also want to investigate @pshihn's suggestion, because we want nice and smooth lines (and good perf/size):

The other thing to keep in mind - you will collect A LOT of points based on just mouse position sampling. You can reduce the points significantly by running through Ramer–Douglas–Peucker_algorithm. I have a javascript implementation here which could be adapted: pshihn/bezier-points:src/index.ts@master#L90

And lastly --- current implem doesn't allow to paint in coordinates negative to initial x/y position:

excal_free_draw_negative

@kbariotis
Copy link
Contributor Author

kbariotis commented May 10, 2020

@dwelle Thank you. I'm investigating the negative coords issue. Not sure what's causing it and I thought someone else could help.

Once I have a good first UX implementation I will start looking at perf/visual issues.

@dwelle
Copy link
Member

dwelle commented May 10, 2020

Yea. It might actually be a good idea to reuse the lines implem as you're doing, because we'll get shape-filling and hitbox testing for free.

@dwelle
Copy link
Member

dwelle commented May 10, 2020

/cc @dai-shi do you see any obvious venue for fixing the bounding-box constraint on drawing to the negative? The implem diverged so much from the original one, that I'm not sure I'll be able to give much pointers here.

@lipis
Copy link
Member

lipis commented May 10, 2020

We should probably have less points.. bigger threshold for adding a new point..

@lipis
Copy link
Member

lipis commented May 10, 2020

Works from the phone as well :)

@dai-shi
Copy link
Member

dai-shi commented May 10, 2020

I can surely help, although it has nothing to do with resizing which I recently modified. There's even an issue with lines.

exdraw39

@@ -2046,6 +2050,7 @@ class App extends React.Component<any, AppState> {
this.setState({
draggingElement: element,
editingElement: element,
multiElement: element,
Copy link
Member

Choose a reason for hiding this comment

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

This is probably causing the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please explain a little bit what that property does? (At some point I was just copy pasting code from other places and I probably messed up somewhere :P )

Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I do not know. 😓 I just assumed this is only the place to cause such an issue. Probably, it's setting the multiElement even when it's only two or less elements. Could you try removing this line and see if the behavior changes? And maybe only add it when it's more than two points. Again, I'm just guessing.

Copy link
Member

Choose a reason for hiding this comment

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

That's not it, although it was causing issues for arrow/line elements, and even some weird jumping for draw element -- so I removed it. Don't know what was the purpose of adding it, but if it's needed for draw elements, we'll need to adapt it since it's very much tied to arrow/line elements.

@dwelle
Copy link
Member

dwelle commented May 11, 2020

Pushed a fix for the bounding box/rendering issue. It was caused because you didn't include draw type in the linear detection during canvas creation. I've reused isLinearElement helper to prevent this from happening again.

@dwelle
Copy link
Member

dwelle commented May 11, 2020

Btw, I just noticed that the #760 was fixed in latest roughjs 🎉, so that's one less thing to worry about in this PR (once #1571 is merged)! false alarm.. I was on architect sloppiness :(.

@dwelle
Copy link
Member

dwelle commented May 11, 2020

excal_warp

While the effect is something the daemons of the Warp would proud of, we need to get rid of it.

@pshihn I noticed that only one of the lines is regenerating when drawing, so I tried setting disableMultiStroke on latest roughjs, and indeed that seems to be the case:

excal_nowarp

It looks like the seed is being applied only to the primary line.

(you may also notice some jumps, but those are almost certainly caused by excalidraw -- when I disabled rendering from element canvases, it went away, so I'll need to take a look at that).

@pshihn
Copy link
Collaborator

pshihn commented May 11, 2020

When drawing multiple point lines, are you setting a new seed for every line segment?

(btw, i do like the single stroke for free drawing with an additional +0.5 strokewidth you did in the dashed case)

@dwelle
Copy link
Member

dwelle commented May 11, 2020

When drawing multiple point lines, are you setting a new seed for every line segment?

Not that I'm aware of, but if we did, it would regenerate both lines, not just the secondary one (which I tested by not passing the seed at all).

(btw, i do like the single stroke for free drawing with an additional +0.5 strokewidth you did in the dashed case)

For lines with many points very close together, yes, but when we implement the algo you proposed to reduce the amount of points, it might be preferable to keep the same render style (but I don't have a strong opinion right now -- would have to see afterwards).

image

But be it as it may, we still want to fix #760.

@pshihn
Copy link
Collaborator

pshihn commented May 11, 2020

There could be an issue in roughjs specific to the curve command which is used for multi point lines. Curves are approximated to a number of points. If the number of points changes, then it affects the randomization with a specified seed.
I'm assuming you are seeing this in multi-point lines/arrows as well (not free draw)
I'll investigate.

Edit: Noticed you linked to #760 which was my question. Missed that .

@dwelle
Copy link
Member

dwelle commented May 11, 2020

Curves are approximated to a number of points. If the number of points changes, then it affects the randomization with a specified seed.

But that doesn't explain why it doesn't randomize the primary curve, if I understand you correctly.

@dwelle
Copy link
Member

dwelle commented May 11, 2020

That said, #760 makes me a bit unsure it's rough's fault (as I checked your code and couldn't find anything -- though that doesn't mean much) since this regeneration occurs on commit-only 🤔.

But we're definitely not regenerating seed.

EDIT: actually, rough is still not off the hook because the difference between moving an uncommitted point and committing it is that after commit there's N+1 points, which is probably significant.

@kbariotis
Copy link
Contributor Author

kbariotis commented May 11, 2020

@dwelle @pshihn I added the simplify method for the curves and while it does make a difference the issue is still visible. (Sorry but I haven't been following your discussion and I feel completely lost, if you have another suggestion let me know)

@pshihn
Copy link
Collaborator

pshihn commented May 11, 2020

@kbariotis yes that issue is independent of simplification.

@dwelle for #760 are you talking about the issue where addin n+1th point changes the penultimate arm of the curve? That is an artifact of curve fitting through n points. Anyways I will investigate all these in detail tonight. I'm sure there's something I forgot to consider wrt seeding.

@dwelle
Copy link
Member

dwelle commented May 11, 2020

@kbariotis any reason to be bundling shortcut changes into this PR? I've got some WIP that will conflict with that 😄. oh, because of tests..

EDIT: is this PR done? We should take a look at the curve simplification first.

EDIT2: oh, forgot you've already done it 😅

EDIT3: that said, that simplification is too aggressive IMO. I'll take a look at it tomorrow if we can make it speed-adaptive.

@dwelle
Copy link
Member

dwelle commented May 11, 2020

Some changes I'd do:

  • use a pen icon instead (https://fontawesome.com/icons/pen?style=solid)
  • move the shape tool either before arrow tool, or after line tool (preferably after line tool so that if people use 5/6 shortcuts, they don't have to relearn them. 7 is less likely used as T is well known).

@pshihn
Copy link
Collaborator

pshihn commented May 11, 2020

@dwelle roughjs 4.3.1 has the fix for this. It should resolve #760 as well
May-11-2020 16-35-09

@vjeux
Copy link
Contributor

vjeux commented May 11, 2020

@pshihn looks great!

@ad1992
Copy link
Member

ad1992 commented May 12, 2020

awesome work @kbariotis. Waiting for this to go live :)
Is it possible to remove the hint "Click to start multiple points, drag for single line" when using free drawing as its not needed ?

@dwelle
Copy link
Member

dwelle commented May 12, 2020

While I'm not satisfied with the simplification algorithm, we can ship it as an alpha implementation and gather feedback while we try to figure out how to improve it.

The 0.5 simplification does stuff like this:

excal_simplify

And decreasing it to 0.4 does this:

excal_simplify_0 4

So there's not much to choose from right now.

@pshihn
Copy link
Collaborator

pshihn commented May 12, 2020

I did not notice the issue you described in 0.5 until I really tried.

One way to address it - You can try buffering after some length - simplify until you reach a certain length or number of points, buffer them. As you keep drawing, only simplify the new points and add to the buffer. Keep adding to buffer as you keep drawing. (A distance of 1.0 might work great here as well)

@dwelle
Copy link
Member

dwelle commented May 12, 2020

Mhm, I may have been inadvertently on a high zoom level. Trying again on 100%, the problem on the 1st GIF can't be easily reproduced. In fact, it now skews more towards the 2nd GIF.

Anyway, we can ship this as is, and tinker with the algo as we go.

Also, I didn't want to sound unappreciative or anything @pshihn. The work you're doing on rough is amazing!

@pshihn
Copy link
Collaborator

pshihn commented May 12, 2020

Sounds good. Have you tried with 1.0? Seems like we jumped from 2.0 to 0.5

@dwelle
Copy link
Member

dwelle commented May 12, 2020

I did -- it feels too rubbery or how to explain it. The lines get simplified in a manner that they often end up in position where you didn't draw them at all, especially when making turns:

excal_simpl_1

And when you make 90° angles, it manifests that behavior from above:

excal_simpl_1_2

The latter should be fixed when we implement the distance heuristic you proposed.

@pshihn
Copy link
Collaborator

pshihn commented May 12, 2020

ah makes sense. Since we are using the curve command to fit these points with roughness there will be artifacts. It still works pretty well with what we have right now. Like you said, can always improve.

@dwelle
Copy link
Member

dwelle commented May 12, 2020

I increased to 0.7.

I also fixed the following:

  • start drawing from 0 distance (unlike for arrows/lines)
  • ensure we remove invisible draws on finalize
  • ensure we close loops under threshold on finalize (for bg)
  • ensure we don't reset tool when shape locked

I didn't set multiElement as that has a lot of arrow/line logic tied to it. Ultimately, we should rename state.multiElement to something that expresses not only that it's a multi-point element, but that it's in a state where you're not holding a mouse down.

@dwelle dwelle merged commit 9ec43d2 into master May 12, 2020
@dwelle dwelle deleted the add-free-draw branch May 12, 2020 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants