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 sketchy transformation #44

Open
wants to merge 52 commits into
base: master
Choose a base branch
from
Open

Conversation

chipjacks
Copy link

Based on excalidraw, rough.js, and this research paper, this allows transforming a collage into a randomized sketchy, hand-drawn version.

The only change I've made to the core modules is to add an API for drawing a curve through a set of points. Admittedly, I don't know much about all the different ways to accomplish this, so I just opted for the simplest implementation I could find.

I've added a way to toggle examples between normal and sketchy rendering and I think the flowchart example best captures the look I was shooting for:

Screen Shot 2021-05-21 at 11 10 57 AM

This is still pretty rough around the edges (hehe), and I'm not entirely sure it belongs inside this package. Maybe would make more sense as a fork or some kind of extension - open to ideas! @timjs any thoughts?

@chipjacks
Copy link
Author

Okay, no rush. I've been working on it a bit more and just finished adding hachures for fills.

PR is getting pretty big - if you'd like I can separate out all the Sketchy logic and then either leave it on a fork, or look into opening a new PR to expose the API I'd need to build this on top of elm-collage but as a separate package.

Right now this is all just a fun experiment, but I am kinda hoping to get it to a good enough place that people could use it if they want.

@timjs
Copy link
Owner

timjs commented Jul 9, 2021

I'm really sorry for the delay, a week turned into a month. A clear sign I should transfer ownership to a new maintainer :-P

It's indeed a quite big PR at the moment. Best would be to split it in a sketchy and in a curves part, but I can imaging this could be quite some work to split it in two separate PRs, and I'm quite happy to merge it this way. It helps that it's just a minor update in the end.

I'll review your changes today and come back to you!

src/Collage/Render.elm Outdated Show resolved Hide resolved
src/Collage/Render.elm Outdated Show resolved Hide resolved
src/Collage/Render.elm Outdated Show resolved Hide resolved
src/Collage/Render.elm Outdated Show resolved Hide resolved
neighbors arr index =
Maybe.map4
(\m1 p1 i p2 ->
((m1, p1), (i, p2))
Copy link
Owner

Choose a reason for hiding this comment

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

This is a trick because Elm doesn't support tuples with more than 3 elements isn't it? Maybe use a record like {prv, cur, nxt, sub} for previous, current, next/sequent, subsequent? Or maybe (prv, cur, (nxt, sub)) when using tuples?

Copy link
Author

Choose a reason for hiding this comment

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

It was because the control points are calculated based on the slope between m1 and p1 and then i and p2`. But I think you're right that it would make more sense just to put them all in order here. Using a record would be a bit more readable but I need to use tuples to be able to destructure the x and y values in the case expression below.

src/Collage/Sketchy/Helpers.elm Outdated Show resolved Hide resolved
Comment on lines +48 to +50
fontFamily : String
fontFamily =
"Caveat"
Copy link
Owner

Choose a reason for hiding this comment

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

Better:

Suggested change
fontFamily : String
fontFamily =
"Caveat"
font : Font
font =
Font "Caveat"

Copy link
Author

Choose a reason for hiding this comment

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

Problem is I'm using this string to load the Google font below. Pretty hacky, let me know if I should take that out.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, there is not a real nice way to solve this, isn't it? Maybe use both?

fontFamily : String
fontFamily =
    "Caveat"

font : Font
font =
    Font fontFamily

src/Collage/Sketchy.elm Show resolved Hide resolved
Comment on lines 169 to 170
render : Flow -> Collage msg
render flow =
Copy link
Owner

Choose a reason for hiding this comment

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

I know it's my own fault, that I used a custom formatter, but I take you didn't change much or anything at all in this function?

Copy link
Author

Choose a reason for hiding this comment

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

Oops, yeah sorry about all the formatting changes, accidentally ran elm-format on this one. I did just change one thing in this function - I made the connecting line stop at the sides of the diamond instead of running behind it where it's visible behind the sketched fill.

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good.

The formatting thing is actually my fault, as I used a custom formatter. (I don't like four space indentations and find all the newlines between case branches way to much noise...) As elm-format is the community standard, I could run it on all source files and make a commit on master, you can do the same and rebase on that? Or would that give us a lot of whitespace errors?

examples/Example.elm Show resolved Hide resolved
@timjs
Copy link
Owner

timjs commented Jul 10, 2021

BTW, what I forgot to write down yesterday before a colleague walked in, the curve function is more a "smooth line" than a general implementation of Bezier curves isn't it? If I understand correctly, it automatically smooths a line, but you can't add control points to have full control over the line. I like this feature, but we should come up with an other name for it!

@chipjacks
Copy link
Author

the curve function is more a "smooth line" than a general implementation of Bezier curves isn't it?

Yeah, that's correct. Maybe rename it to smoothPath?

On a related note, if we added a real bezier curve API as discussed in #8, then I could improve the browser performance significantly by rendering the hachure fills as one svg path using a bunch of M x y commands, instead of a list of separate path elements as I'm currently doing. Probably just leave that for a separate PR though.

@chipjacks
Copy link
Author

Thanks for the review @timjs! I've made the changes you requested.

Copy link
Owner

@timjs timjs left a comment

Choose a reason for hiding this comment

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

Added two minor comments.

As written above, the formatting thing is actually my fault, as I used a custom formatter. (I don't like four space indentations and find all the newlines between case branches way to much noise... A simple function in Elm can easily fall off my screen!)

As elm-format is the community standard, I could run it on all source files and make a commit on master, you can do the same and rebase on that? Or would that give us a lot of whitespace errors?

src/Collage/Render.elm Outdated Show resolved Hide resolved
src/Helpers.elm Outdated Show resolved Hide resolved
Comment on lines +48 to +50
fontFamily : String
fontFamily =
"Caveat"
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, there is not a real nice way to solve this, isn't it? Maybe use both?

fontFamily : String
fontFamily =
    "Caveat"

font : Font
font =
    Font fontFamily

@chipjacks
Copy link
Author

As elm-format is the community standard, I could run it on all source files and make a commit on master, you can do the same and rebase on that? Or would that give us a lot of whitespace errors?

Yep, I tried rebasing but it results in too many whitespace errors. An alternative could be to run elm-format on both branches separately - it looks like that leads to a clean diff without all the unrelated whitespace changes.

This was referenced Jul 29, 2021
let
text =
fromString label
|> Text.typeface (Text.Font fontFamily)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
|> Text.typeface (Text.Font fontFamily)
|> Text.typeface font

@timjs
Copy link
Owner

timjs commented Aug 4, 2021

Hi @chipjacks,

Sorry for the delay, was on a holiday ⛰🥾

Merged your other two pull requests. Next steps will be to rebase this one on current master, and I'll merg it!

Thanks for all the great work and the constructive discussions 😄

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.

2 participants