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

Edge Shading #14

Merged
merged 20 commits into from
Dec 19, 2023
Merged

Edge Shading #14

merged 20 commits into from
Dec 19, 2023

Conversation

OscarSaharoy
Copy link

Hi, hope this project is still active :) I love jp2a and I had an idea which I think could be cool.

I added a few changes which allow the user to set an --edge-threshold. Anywhere that the image gradient is above this value, a directional character like /|\= is inserted, which can help highlight edges. I think the implementation could maybe be a bit better so any ideas appreciated!

Here is a demo, you can see the whiskers kind of get highlighted a bit:

$ ./src/jp2a --color --term-width --edge-threshold=0.3 ~/storage/pictures/Screenshots/Screenshot_20231126-174650_Gallery.png

IMG_20231128_200651

IMG_20231128_203003

Copy link
Owner

@Talinx Talinx left a comment

Choose a reason for hiding this comment

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

Great idea and implementation!

I got some more ideas (if you want to implement more):

  • An option to suppress non-edge output would be neat, i.e. like an edge-detector.
  • Support for different common edge-detection kernels could also be added.

(Also one test (make test, test ( 9) (width, border)) now fails. If you want you can look into this, otherwise I'll fix that.)

src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
@Talinx Talinx changed the base branch from master to develop December 5, 2023 13:11
@OscarSaharoy
Copy link
Author

Thanks so much for taking a look :) the test failure was due to accessing outside the image array but should be fixed now. And thanks for the edges only suggestion! it works really nice:

Screenshot from 2023-12-10 13-39-39

For the alternative edge detection kernels, I did try a couple of them but the simple one I have used seemed the best. I think the subsampling of the initial image means that the edge detection is already kind of awkward as edges are blurred out a bit, so to take advantage of different edge kernels it would probably make more sense to apply to kernel before subsampling. Maybe that can be a future feature :) The test script is also very cool, I can add some tests for edge shading if you like? But I guess you might have a better idea of what to add than me.

@Talinx
Copy link
Owner

Talinx commented Dec 12, 2023

Makes sense that other kernels do not look really different for ASCII art.

Adding three little test (one with and one without --edges-only and one with both --edges-only and --invert would be nice, using tests/jp2a.jpg sounds good to me.

Two little things are still missing:

  • bash completion for the new command options (completion/bash/jp2a)
  • adding the new command options to the man page (man/jp2a.1)

Also when using --edges-only and --invert, empty space/spaces are replaced with M. Is this intended? (This happens because pos is set to 0 (due to --edges-only), which then selects the last char from the ASCII chars (M) because the first pos is switched with the last (due to --invert).)

This is coming together very nicely ;)

@OscarSaharoy
Copy link
Author

The man page and autocomplete should be updated now :) and I also added a commit to prevent a space being added when you autocomplete an option that ends in =

Before I do the tests I am just wondering about the behaviour with --edges-only and --invert - I think the current behaviour is ok because it gives the user the option to choose whether the first or last character in the palette is used to fill the rest if the image, which seem like the two logical choices. With certain custom palettes this could be useful? If there is a better way to handle it though I will change it and then make the tests :)

@Talinx
Copy link
Owner

Talinx commented Dec 16, 2023

The current behavior for --invert looks good to me. Just wanted to make sure this code path is thought about.

@OscarSaharoy
Copy link
Author

Ok perfect, the tests should be there now :)

@Talinx
Copy link
Owner

Talinx commented Dec 17, 2023

Awesome! Unless you want to add something else to this PR it's ready to merge.

@OscarSaharoy
Copy link
Author

Thanks so much! Sounds good to me, I really enjoyed working on this and learned a lot :)

@Talinx Talinx merged commit 1799f48 into Talinx:develop Dec 19, 2023
1 check passed
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