gadenbuie / xaringanExtra

:ferris_wheel: A playground of enhancements and extensions for xaringan slides.
https://pkg.garrickadenbuie.com/xaringanExtra
Other
448 stars 36 forks source link

Adds scribble: draw on xaringan slides #87

Closed mattwarkentin closed 3 years ago

mattwarkentin commented 3 years ago

Hi @gadenbuie,

This PR is related to #83. As of now this extension is pretty feature-complete based on the suggestions in #83, however, there is one fatal flaw regarding the canvas resizing that I haven't been able to solve, described below.

Features

fabric.js

Initially, fabric.js was used as the canvas back-end. However, since we wanted to support proper erasing ability, this caused some challenges. The benefit of fabric.js is that it turns your drawing into proper paths/vectors. This makes for nice looking lines. But the downside is that it is very tricky to erase since the line is a fully connected path, and not a series of sequentially connected points. To erase a path, you would have to clip the path and then somehow remove the disconnected component. This is why the default "erase" functionality for fabric.js is just painting over the path with white paint (or whatever colour the canvas is). This won't work for our purposes since that would just obfuscate the slide content underneath the canvas.

Thankfully, the default <canvas> has JS support for proper erasing. I thought proper erasing was an important enough feature that I made the choice to build my own drawing tool, rather than reverse engineer an erasing tool for fabric.js.

Issues

Currently, the canvas works pretty much as expected on "full screen" size slides. When the slides are resized (typically to smaller sizes), the offset is noticeably awful and the drawing is no where near the cursor. I have been spinning my wheels trying to fix this. I have tried several approaches, none of which helped.

Some possibly helpful comments:

Anyway, I'm hoping you will be able to help provide insights that lead to a fix. I will keeping working on this draft PR in the meantime. I look forward to your feedback and I am happy to make any revisions.

CC @rpruim and @LauraRK

gadenbuie commented 3 years ago

This is looking really good so far! Some thoughts after reading and trying this out.

The benefit of fabric.js is that it turns your drawing into proper paths/vectors. This makes for nice looking lines. But the downside is that it is very tricky to erase since the line is a fully connected path, and not a series of sequentially connected points. To erase a path, you would have to clip the path and then somehow remove the disconnected component.

I hate to say this now that you've re-written things with canvas, but this might not be a problem. I think that erasing part of a path makes sense if you're using a drawing app, but that's not something you want to do when your scribbling on slides.

I use an app called GoodNotes that has a pretty simple drawing interface, and one of the things that I love about it is that the eraser removes the entire path of whatever it intersects. I think this kind of erasing would be very nice. And if it's possible with fabric.js then it'd also be nice to have the better-looking lines.

https://user-images.githubusercontent.com/5420529/106989390-11416b80-6740-11eb-8f9c-5c2cbb7c3fc4.MP4

When the slides are resized (typically to smaller sizes), the offset is noticeably awful and the drawing is no where near the cursor.

  • One odd observation which may be insightful, on full-screen sizes, the correspondence between the cursor and drawing is perfect in the top-left corner, and gets worse as you move down and right

This makes sense. remark uses CSS transform: scale(ratio) and adjusts the ratio value dynamically when the slide resizes. The transform origin is the upper left corner so things up there will look fine. I think you could fix it by taking into account the scaling when you draw on the canvas, but...

Rather than doing linear algebra, I'd recommend putting the canvas somewhere else, e.g. outside the .remark-slide-scaler. I made a proof of concept in https://github.com/gadenbuie/xaringanExtra/commit/2f9ebeafabc072a34eb572955e1a64e69d9e0cb0, you can pull those changes into your branch if its helpful.

Kapture 2021-02-05 at 01 01 05

The downside of this approach is that resizing the slides will resize the canvas and destroy and drawings. That might be unavoidable with canvas without adding a real headache. It seems like something fabric might be able to do?

Other quick notes

mattwarkentin commented 3 years ago

Thanks for the great feedback, Garrick!

I will look further into what is achievable with fabric.js (e.g. path erasing) and then we can make a decision about which direction we should go in.

In the meantime, I will work on removing the fontawesome dependency, adding an example presentation for testing/feature demonstration, and general code maintenance.

I like your JavaScript style and appreciate the helper functions

I'm glad to hear it. This is pretty much the first thing I've ever built in JS, so there have been lots of mistakes made and a lot of learning on the fly, but it has been a good experience. I've definitely strengthened my JS abilities.

gadenbuie commented 3 years ago

There are 3 actions: drawing, erasing, and clearing; and there are 2 states: canvas shown or canvas hidden.

  • Clicking the green check will disable any action, and keep the canvas visible. Clicking the red X will disable any action and hide the canvas. The markup persists on a hidden canvas, unless the canvas is explicitly cleared. This allows users to hide/show their drawings, as desired.

I'd frame it as four states:

  1. Canvas visible
  2. Drawing on canvas
  3. Erasing on canvas
  4. Canvas hidden
           +----> drawing
           |         ^
 visible <-+         |
    ^      |         v
    |      +----> erasing
    |
    v
  hidden

but we can make that three buttons that have "toggle" state:

  1. Show/hide canvas
  2. Drawing/(nothing)
  3. Erasing/(nothing)

Turning on drawing jumps to the visible state. Erasing and drawing toggle the other off if previously on.

A final, fourth button would be "clear" which erases the canvas, regardless of state.

Another thought here: jumping from hidden to drawing could also clear the canvas.

I'd like to explore simplifying it a bit more though to say that you can't "hide" the canvas, you can only clear it. In that case the states would be

  1. Drawing
  2. Erasing
  3. Nothing

and the actions would be

  1. Toggle drawing
  2. Toggle erasing
  3. Clear canvas

Hiding the canvas is also necessary to allow users to interact with normal slide content (e.g. widgets, clipboards, etc.)

I think you can use pointer-events: none; to let events pass through the canvas layer. I'm not sure how that interacts with touch mode. Another option might be to add and remove the canvas-related event listeners. If it proves to be tricky I can help here (and this would be a good case to include in your demo slides to make sure you can click through the canvas).

mattwarkentin commented 3 years ago

I like these ideas for simplifying the UI. I guess we need to decide on whether we want the ability to hide the canvas while keeping the drawing in tact (current behaviour), or we decide the canvas is always "on top" but we allow users to "exit" from drawing/erasing mode so they can interact with content underneath. Do you see a use-case for temporarily hiding the mark up? I guess thats the root question.

So we can get it down to three buttons

  1. Draw button - While in erase mode, clicking this turns on draw mode and disables erase mode. While in draw mode, clicking this again enters "nothing" mode.
  2. Erase button - Same as above, but opposite
  3. Hide/show/exit/clear button - This button could either hide/reveal the canvas, exit draw/erase mode so we can allow clickthroughs to the content underneath, or clear the content and enter "nothing mode".

Thoughts?

gadenbuie commented 3 years ago

Provided the user can interact with the slide below the canvas while not in drawing or erasing mode, I think we should only have a clear option. This will make the feature easier to design and easier to use. Down the line I could see us needing a way to persist the drawings for later viewing or printing, and we could tackle that then. But for new let's stay small and easy to use.

LauraRK commented 3 years ago

I agree simpler is better. If someone has something more complex they could make two copies of the same slide and go back and forth if need be. Also just to double check what is the current way to test?

Thanks!

Sent from my iPhone

On Feb 5, 2021, at 5:02 PM, Garrick Aden-Buie notifications@github.com wrote:



Provided the user can interact with the slide below the canvas while not in drawing or erasing mode, I think we should only have a clear option. This will make the feature easier to design and easier to use. Down the line I could see us needing a way to persist the drawings for later viewing or printing, and we could tackle that then. But for new let's stay small and easy to use.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/gadenbuie/xaringanExtra/pull/87#issuecomment-774312446, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ANJXVJJ4D4FEZZISJ5E7YMLS5RTI3ANCNFSM4XDWZ36Q.

mattwarkentin commented 3 years ago

Also just to double check what is the current way to test?

@LauraRK You can test a mostly working but buggy version by installing remotes::install_github("mattwarkentin/xaringanExtra"). But the interface/functionality is going to change over the next several days as I work on implementing Garrick's suggestions.

rpruim commented 3 years ago

I will look further into what is achievable with fabric.js (e.g. path erasing) and then we can make a decision about which direction we should go in.

Just chiming in to say that I'd also be happy with path erasure and like the better paths that fabric provides.

Would this also provide the possibility of an undo button to remove the most recent stroke/path? That's a pretty common operation when something doesn't turn out as planned and can help avoid removing things that one would like to keep.

rpruim commented 3 years ago

BTW, I like the ability to hide the drawing controls (and get them back with 'd'). Is there any way to make that work on a touch device? If so, we could have the controls hidden by default and require action to reveal them.

LauraRK commented 3 years ago

I got a chance to test it out today and as of now I can't get it to be functional using my windows machine, either in full screen or otherwise. I have a touch screen windows laptop, and I also have a bigger monitor. I also could not get the eraser to work at all, but that might be related to me not knowing where to try and erase given there is a misalignment.

mattwarkentin commented 3 years ago

@LauraRK This feature is still under rapid development. I would definitely recommend holding off on testing/using this feature, for now, until we settle on an API and the feature stabilizes.

mattwarkentin commented 3 years ago

@gadenbuie So I took a step back to see what was achievable with fabric.js before spending too much more time on the current implementation. I would love to get your feedback on this toy implementation (fab.zip) and we can decide if we should use fabric.js moving forward (I think we probably should, it opens up a lot of future features also). You can generally ignore the code, it was slapped together for proof-of-principle.

I look forward to your thoughts.

rpruim commented 3 years ago

I like the demo and agree that it seems like fabric.js offers some nice features.

It would be nice if there were something visible to show the radius of the eraser tool, but I like stroke-wise erasing just fine.

The ability to move or rescale drawings (which fabric makes really easy) is a possible bonus.

So... I'm in favor of moving forward with fabric.js.

rpruim commented 3 years ago

In the demo, sometimes strokes don't erase. I haven't figured out what conditions will let me reliably reproduce the non-erasing. So for now, just something to watch out for.

mattwarkentin commented 3 years ago

In the demo, sometimes strokes don't erase. I haven't figured out what conditions will let me reliably reproduce the non-erasing. So for now, just something to watch out for.

Yes, I've noticed this too. I think this is related to the fact that while "erasing" you are actually still drawing a transparent path. With enough erasing, I think the observable paths are somehow obfuscated by the transparent paths. If I'm right, this is definitely a simple fix. Simply need to remove all transparent paths on "mouseup". Thus the canvas won't get bloated with invisible lines.

mattwarkentin commented 3 years ago

It would be nice if there were something visible to show the radius of the eraser tool, but I like stroke-wise erasing just fine.

Please try this (fab.zip) and let me know if you think this is a reasonable solution. We can allow the user to control the eraser width and stroke colour, if desired.

rpruim commented 3 years ago

I think I would prefer just a circle rather than the path, and I would erase things as soon as the circle hits the path.

I would set the radius smaller, but as long as that is configurable, that's not a big issue.

Colors are tricky because we can't now what the background color will be, so probably good for this to configurable as well.

mattwarkentin commented 3 years ago

I think I would prefer just a circle rather than the path

I don't think this is possible.

I would erase things as soon as the circle hits the path.

This should be the current behaviour. As soon as the "eraser" gets near a path point, the path should be immediately erased. Keep in mind that the amount of points there are along a given path is dependent on the speed the path was drawn. A slowly drawn path has more points, and so it should be "easier" to get near a point when trying to erase, and vice versa. This is why erasing in fabric.js is hacked together and why we need to use a liberal tolerance value to match the eraser and path points.

But yes, all pen and eraser features will be configurable.

rpruim commented 3 years ago

Ah, I misunderstood "points". In my way of thinking there are "control points" that are being interpolated to give "visible points". I was thinking you were able to work with visible points, but now I understand that you are working with control points. That clarifies a few things in my mind.

gadenbuie commented 3 years ago

@mattwarkentin The fabric.js implementation looks awesome! I think this is definitely the way to go. I'd recommend focusing on the states and the JavaScript to glue it all together. Don't worry much about the UI elements beyond what's minimally required, we'll work on that second. The limitations you mentioned all sound reasonable and are a perfect start. We can always tweak once we have the pieces in place.

gadenbuie commented 3 years ago

@mattwarkentin I can't believe I just now stumbled on this, but remarks' slideshow object has .pause() and .resume() methods (docs):

// Suspend/resume remarks process of keyboard and touch events for custom builds, etc...
slideshow.pause();
slideshow.resume();
mattwarkentin commented 3 years ago

Wow, great find. I can't believe I also didn't stumble upon this when reading over the remark wiki a million times. Things are progressing well and we have eliminated a lot of fragile code by using fabric.js and slideshow.pause()/resume(). Will commit changes soon for review.

LauraRK commented 3 years ago

I wanted to just say that I very much agree with the simple for now, get it working well enough, then worry about gui features later, way of doing things. Thank you again.

mattwarkentin commented 3 years ago

Sorry, I didn't have much time the last few weeks to work on this so these updates took longer than I anticipated. @gadenbuie, I think this is ready for review.

Noteworthy changes:

I look forward to your feedback.

LauraRK commented 3 years ago

Hi this is great so far. Thank you so much. I tested it with my laptop using a "dell pen" and so far so good. Better than writing on powerpoint slides and I love the change the default color feature.

As it currently works it seems that I need to click with my mouse the drawing off to use the page up and page down or arrow keys or mouse scroll to go between slides. Is this true or is there another hot key that works? D hides or unhides, but if you hide without turning drawing off you can't scroll. Is there also a key to turn off drawing so I can scroll? If no that is OK, I can click it off, I just wanted to make sure I understood how this is working. I like it already and really like that I can pick a default color. Thank you.

mattwarkentin commented 3 years ago

Is there also a key to turn off drawing so I can scroll?

Currently, while in draw or erase mode, we disable the ability to navigate slides (slideshow.pause()). This was primarily a consideration for touch devices, because we need to be able to touch the screen to click buttons and draw/erase without causing slides to change (the default touch behaviour for remark.js).

As of now, the only way to resume slide navigation (slideshow.resume()) is to exit draw/erase mode by "turning off" whichever of draw or erase are currently selected.

LauraRK commented 3 years ago

Got it. thank you. That makes sense.

Sent from my iPhone

On Feb 23, 2021, at 5:00 PM, Matthew T. Warkentin notifications@github.com wrote:



Is there also a key to turn off drawing so I can scroll?

Currently, while in draw or erase mode, we disable the ability to navigate slides (slideshow.pause()). This was primarily a consideration for touch devices, because we need to be able to touch the screen to click buttons and draw/erase without causing slides to change (the default touch behaviour for remark.js).

As of now, the only way to resume slide navigation (slideshow.resume()) is to exit draw/erase mode by "turning off" whichever of draw or erase are currently selected.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/gadenbuie/xaringanExtra/pull/87#issuecomment-784542636, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ANJXVJPCF6KJBC7CAI74LADTAQQPLANCNFSM4XDWZ36Q.

mattwarkentin commented 3 years ago

Minor type fix

🤦‍♂️

mattwarkentin commented 3 years ago

@gadenbuie I think this revision is a major step in the right direction. I have completely refactored the JS code to define the Scribble class. This creates a much nicer and self-contained JS object. I also refactored and renamed a lot of the class methods to be more evocative and modular. I look forward to your feedback.

LauraRK commented 3 years ago

I tested out the new scribble version and it works very smoothly on my touch screen laptop and on my ipad with the apple pencil. I really like the eraser and how quickly it erases. For my purposes the tool works very well.

rpruim commented 3 years ago
  1. You had a double click method at one point and I liked the idea. Double click to turn off drawing mode might make it easier to step out of drawing. Edit: actually I'm not sure about this. I wonder if it would get tripped up by people trying to draw dotted lines.

Just a personal comment here: I find that I often have to disable this sort of thing when using a pencil on my ipad. My "attack" often registers as a double click. That's probably a quirk in how I write, but I'm probably not the only one who has this issue. As mentioned above, dotted lines might also be an issue.

mattwarkentin commented 3 years ago

@gadenbuie Okay, I think I've addressed this round of changes. A few additional thoughts:

Also, in this revision I added some code that makes it so the "zoom" level of the canvas scales with the .remark-slide-scaler. This fixes the issue whereby the drawn paths wouldn't change when resizing the presentation. Now, the paths scale with the slide, and the eraser still works. I'm interested to hear your thoughts.

mattwarkentin commented 3 years ago

As suspected, the undo/redo mechanism was very simple to implement, so it's an easy add, if we think it's worth it.

gadenbuie commented 3 years ago

As suspected, the undo/redo mechanism was very simple to implement, so it's an easy add, if we think it's worth it.

I'm really excited about both the drawing scaling and the undo/redo! Please go ahead and add that in 😀

mattwarkentin commented 3 years ago

Done! I highjacked the left/right arrow key for undo/redo, respectively. Since we disable slide navigation during interactive mode anyway, those keys were available and I thought they would be intuitive to use (open to suggestions). The cache of removed paths for a given slide is destroyed when you leave the slide - this was a simplicity/design choice, but it wouldn't be hard to implement a long-term memory for the cache.

After drawing a line, press left-arrow to undo, and then the right-arrow to redo. After erasing a line, press the right-arrow to add it back (in this sense, erase is almost like an undo, and redo adds it back). Exiting draw/erase mode will return the arrow keys to performing slide navigation only.

gadenbuie commented 3 years ago

@mattwarkentin Apologies that I'm pushing back to your master branch, but I made a few changes, primarily around refactoring the UI. This extension is awesome and I'm really happy how it's going. Thanks for all of your work on this!

Here's a quick demo of what it looks like now:

https://user-images.githubusercontent.com/5420529/110147209-64dbbf00-7da9-11eb-8a59-bca3a0afc2b2.mp4

gadenbuie commented 3 years ago

@mattwarkentin This is almost ready for prime time! I made a few more minor final changes, and I think all of the R and JavaScript components are in now in a good place. I still need to update the xaringanExtra docs page and the demo slides.

If you have a chance to review the changes, I'd appreciate a final look! Hopefully the updates are clear from the commit messages, but let me know if you have any questions.

mattwarkentin commented 3 years ago

Wow, the UI looks much better than what I had made. I have a lot to learn. Sure, I'll review the changes and add any comments if anything isn't clear or needs review.

LauraRK commented 3 years ago

Thank you for going above and beyond with this amazing addition. My sincere gratitude to @mattwarkentin and @gadenbuie for making my teaching life easier.

mattwarkentin commented 3 years ago

This was fun. I learned a lot and have a lot to learn. I appreciate that it takes more time/effort to review code and provide feedback than to just write the code yourself, so thanks @gadenbuie for allowing me to be part of the process.

LauraRK commented 3 years ago

@mattwarkentin and @gadenbuie I remade my slides and now I can't see the drawing features. The last iteration with the old interface they worked just fine.
Now I reinstalled and I no longer have the drawing tools. Is there some other feature I need to have turned on as well?

below is how I am calling use scribble. Thank you .

xaringanExtra::use_scribble(pen_color="rgb(61,255,232)")
mattwarkentin commented 3 years ago

@LauraRK I am able to reproduce the error. It is a bug we need to fix. In the meantime you can get around this issue by using:

# Note the spaces
xaringanExtra::use_scribble(pen_color="rgb(61, 255, 232)")

@gadenbuie The RGB regex at line 232 is sensitive to whitespace and throws an error at line 234 because digits is empty: https://github.com/gadenbuie/xaringanExtra/blob/81a7b220ef9062eac28d20a348aadf65a744ee22/inst/scribble/scribble.js#L227-L240

LauraRK commented 3 years ago

Thank you. I realized after I sent this that I should have tested it deleting the special pen_color. I appreciate you taking the time to reproduce the error and diagnosis it and explaining the issue. I will just put in the spaces. :) I think I was confused because I believe this same code worked previously.

gadenbuie commented 3 years ago

The "real" problem is here https://github.com/gadenbuie/xaringanExtra/blob/81a7b220ef9062eac28d20a348aadf65a744ee22/inst/scribble/scribble.js#L112 where color is user input, which actually traces back to https://github.com/gadenbuie/xaringanExtra/blob/81a7b220ef9062eac28d20a348aadf65a744ee22/inst/scribble/scribble.js#L47 In all the other places we're getting color from window.getComputedValue() or from the color input so we know (or can hope) it has consistent formatting.

gadenbuie commented 3 years ago

@mattwarkentin and @gadenbuie I remade my slides and now I can't see the drawing features. The last iteration with the old interface they worked just fine. Now I reinstalled and I no longer have the drawing tools. Is there some other feature I need to have turned on as well?

below is how I am calling use scribble. Thank you .

xaringanExtra::use_scribble(pen_color="rgb(61,255,232)")

Hi @LauraRK — So I have good news and bad news. The good news is that I've fixed this issue! The bad news is that we now require that pen_color specifically be provided in hexadecimal format, e.g. #3DFFE8" instead of "rgb(61,255,232)". Fortunately you can convert between the two using the R function rgb(). Just remove the quotes around the string and add maxColorValue = 255 to the rgb() call. Thanks again for your testing and feedback and sorry for the inconvenience.

LauraRK commented 3 years ago

Please do not apologize, I can use the hexadecimal formats. I have been so happy with this tool. It really makes my teaching workflow easier. I like that to erase everything I can just reload. I like that I do not have to print to pdf. It saves me having to think so much about what I am doing, which has been a challenge for me with teaching online. I even made myself a little "whiteboard" to keep bookmarked on my browser so I can illustrate easily as needed without unsharing my screen and using the zoom whiteboard. Whiteboard is an example. I can bring it up easily when I am working in Rstudio or during a presentation or really at anytime.