-
Notifications
You must be signed in to change notification settings - Fork 47
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
Display ascii art with ansi colors. #80
Conversation
This is a good feature, but one thing to consider is the speed at which the file is converted from a bitmap to characters we can print, so how's the performance looking? Another major drawback is having to bring termwiz as a dependency, which in and of itself, brings an absurd amount of dependencies. I think a major struggle for Macchina and libmacchina right now is the annoying build time that can probably be reduced if we didn't use as many dependencies as we currently do. After merging the custom-ascii PR (#78), I do not see this as a necessity since the user can import their own ASCII art after having converted it from image to text using jp2a or any other program, we also already provide them with basic color customization and I think a key feature is to extend that customization with ANSI color code parsing. |
Performance definitely takes a hit. |
Yes It's definitely a drawback. |
That would be a lovely contribution 😄 |
So I tried using the ansi_parser crate but It adds 15-20ms of latency just to parse the ascii with colors and doesn't even convert colors to any higher type so I'm writing a parser from scratch. It should have 4-bit / 8-bit / TrueColor support. It's probably going to take a while since I'm going in blind. |
That stucks!
Make a separate crate for this, as other people might find it super helpful too :) |
Okay so I have completed a draft of the parser. Here is the parser ansi-to-tui Benchmarks |
Absolutely terrific! Benchmarks were almost untouched. So, is the file converting say {c1} to a certain ANSI value or a converting ANSI directly to |
Currently its parsing a byte sequence like |
However this currently assumes that only there are only ansi colors and not any other ansi byte sequence. However jp2a should always give ansi color only outputs. Performance can probably be improved further with a bit of tweaking. |
I was taking a look at the Is it currently not possible to color-code multiple characters at once? |
@CuriouslyCurious might be interested in this lovely work you've made as he's done some research to try and make this possible, he might be able to give you a hand so you can maybe work on this together and deliver an all-round faster and better solution. |
Yeah that is one problem I'm facing. |
I'd love to help you on this but I've got a uni project that involves PHP (uugh..) I'm required to make. |
Haha ! I also have a project going on for college, I was thinking of porting it over to rust from python. |
If it's a small project I'd say go for it! |
I would but then people working with me would have to learn rust and that's quite possibly the biggest challenge. |
Oh, you're not solo, yeah it'd be hard to convince them, but give it a try! Maybe they like the things Rust brings to the table. |
Seems like @uttarayan21 has mostly got it under control from what I can see. A bit miffed that he was so fast, as I was making some slow but steady progress on the cansi crate 😛. His code isn't as simplistic as that crate, but it can surely be iterated upon as it seems to handle stuff correctly. I'll keep going with the cansi crate PR though as I think it may be useful for other projects in the future. Rewriting Great job, @uttarayan21! |
Haha, thanks, @CuriouslyCurious I am actually refactoring the code to make it simpler. I, previously, hadn't heard of the
|
Okay so I did a bit more work and now it supports UTF-8. Also utf-8 parsing can be done using String::from_utf8 or But since most of the UTF-8 sequences are smaller than 64 ( usually 1 with |
Beautiful stuff, you're getting there! :) |
Only to do micro optimizations now.
I think that at the moment, it's not very intuitive, and its very repetitive. We need to make it so users can use {cX} to configure the color of the ASCII instead of the actual ANSI values, and we also need to make it so the parser can apply the colors for not just one character, but multiple characters and entire lines. This will most likely improve performance too as we're converting entire lines in some cases instead of characters one by one. |
It'd be helpful to have that explanation in the script for new users so they know what the script does :)
I think that's more fitting, yeah. |
Okay I added a comment to the script. Also is there anything else I should clean up before I push ? |
Also as of now macchina panics if a file has more than 50 lines / 500 columns since that is the limit of the buffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed you took out --custom-ascii-color
, was it causing issues?
Also as of now macchina panics if a file has more than 50 lines / 500 columns since that is the limit of the buffer.
How about not displaying it if it exceeds 50 lines instead of panicking?
Yeah since styling is entirely dependent on the ansi codes and the codes are converted to color in the
Then we'd have to run a line count on the text file before it is passed to ansi-to-tui since it converts all of the lines to |
Can you merge the latest changes from main? The macOS issue should be fixed :)
Well, |
Remove useless files. Renamed macchina-ascii.sh to macchina-video.sh
The ansi codes can be pretty easily removed from the color by just using sed / awk, IMO which would be much more efficient than removing them at runtime every single run. Also removing them is pretty easy in rust too, since they are 0 width unicode characters So I'm thinking |
This comment has been minimized.
This comment has been minimized.
That's true, but we're trying to cover the edge cases so our users don't have to.
So pass the buffer to
Exactly 😄 |
It's compiling the old version of libmacchina, libmacchina 0.3.0 should work fine. |
Ah It didn't show up in
Yeah pretty much. |
Is EDIT: Make sure to upgrade libmacchina to 0.3.1, which adds support for querying battery health (linux only though) |
It's an external crate. Also I'll write an implementation tomorrow, should be pretty easy.
Oh yeah sure. |
Well, thanks a lot for these, gonna check each one out :)
Awesome, we should be set after that and ready to merge, thanks! 🎊 |
What obstacles did you encounter?
Awesome! I can't wait to have my hands on this :) |
I totally forgot about the fact that the escape sequences are not all zero width unicode so there are characters like [ 0..9 m , etc. that also need to be removed. Also the fact that I've been forcing myself to switch from |
Ah, okay. When you first mentioned their width always being zero, I had my doubts. But seeing as I've never ventured too far into ANSI or made a parser, I took your word for it. Glad you figured it out :)
I tried to do this twice in the past, and I just couldn't... |
Same here, this is my third time trying to switch. |
Add checks for large files.
@grtcdr can you check ? I think its done. |
Everything looks good! And it seems like performance wasn't affected. Thanks for your contributions @uttarayan21 ❤️ , it was a pleasure seeing custom ASCII being implemented using a proper parser, I appreciate your efforts and I hope you stick around with us 🥳 |
Haha it was definitely fun working on it.
I'll definitely stick around for a while :)
…On 02/05/21 01:23PM, Taha Aziz Ben Ali wrote:
@grtcdr can you check ? I think its done.
Everything looks good!
And it seems like performance wasn't affected.
Thanks for your contributions @uttarayan21 ❤️ , it was a pleasure seeing custom
ASCII being implemented using a proper parser, I appreciate your efforts and I
hope you stick around with us 🥳
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.*
--
Uttarayan Mondal
|
This is a optional feature that will add option to use.jp2a
as the ascii backendThis will add auto color support as well as a way to use jpg/png images for the ascii art.This will add color support to ascii images made using jp2a or similar tools.
This is an initial draft, will need more work.
Screenshot