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

Feature: width argument to calculate scale from number of available columns #6

Merged
merged 3 commits into from
Oct 26, 2022

Conversation

giulio-Joshi
Copy link

Hello, its me again 😃

I'm using your converter and found myself in this situation: I wanted to aartyze a bunch of pics from different size, all to an ascii file the same width.
This feature come out naturally and gave the occasion of testing a small part of the code.

Do you think this could help you?

giulio-Joshi added 2 commits October 25, 2022 21:07
Will calculate scaling based on parameter-specified columns number.
@0x61nas
Copy link
Owner

0x61nas commented Oct 26, 2022

Nice feature @giulio-Joshi, I Think it will be useful ❤️

@0x61nas
Copy link
Owner

0x61nas commented Oct 26, 2022

I'm glad you liked the tool, And thanks to you for tests

@0x61nas 0x61nas self-requested a review October 26, 2022 16:01
@0x61nas 0x61nas added the enhancement New feature or request label Oct 26, 2022
src/args.rs Outdated Show resolved Hide resolved
/// Determine how much scale to use in presence of `width` parameters,
/// otherwise returns regular `scale` parameter per default behaviour
///
fn calculate_scale(args: &Arguments, dimensions: (u32, u32)) -> u32 {
Copy link
Owner

Choose a reason for hiding this comment

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

You can make this method as inline method using the #[inline] suggests, This will can provide small but easy speed win.
https://nnethercote.github.io/perf-book/inlining.html

Copy link
Author

Choose a reason for hiding this comment

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

No problem, even if actually is called just once per run.

Could gain more performance if we read the image from a file, or from stdin (for better bash integration).

Copy link
Owner

Choose a reason for hiding this comment

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

I understand reading the image from a file, but what you mean with reading it from stdin?

Copy link
Author

Choose a reason for hiding this comment

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

That would mean like how the -@ options works for zip program:

ls *.jpg | zip image_gallery.zip -@

All *.jpg files get streamed to zip and each one gets added to image_gallery.zip
But that would mean changing some of the program parameters.

README.md Show resolved Hide resolved
Copy link
Owner

@0x61nas 0x61nas left a comment

Choose a reason for hiding this comment

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

Nice addition thanks, but there are some minor tweaks

* Documentation consistency
* Method inlining
@giulio-Joshi
Copy link
Author

Yes of course, changes incoming.

@0x61nas 0x61nas self-requested a review October 26, 2022 19:21
@0x61nas 0x61nas merged commit 9a7730d into 0x61nas:master Oct 26, 2022
@giulio-Joshi giulio-Joshi deleted the columns_arg branch October 26, 2022 19:51
@0x61nas 0x61nas mentioned this pull request Nov 28, 2023
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants