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

Refactor package structure #58

Merged
merged 32 commits into from
Aug 6, 2022
Merged

Refactor package structure #58

merged 32 commits into from
Aug 6, 2022

Conversation

t-bltg
Copy link
Collaborator

@t-bltg t-bltg commented Dec 20, 2021

@johnnychen94, this is my first shot at #52 for this evening :

] test ImageEncoding and ] test ImageInTerminal pass.

Am I going in the good direction ?

EDIT: about the workflow here, how can we test the CI if the new sub-package ImageEncoding is not registered, and how can we test the sub-package in the CI ?

TODO:

- [ ] add tests for #49

  • add more backends

EDIT: 'ERROR: UndefVarError: ImageEncoding not defined' on windows CI only, any idea why ??
==> solved with 'defaults:
run:
shell: bash'

@t-bltg t-bltg force-pushed the design branch 12 times, most recently from 0141349 to f1b1ed1 Compare December 20, 2021 22:53
Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Since the current version is mostly code-moving progress, I only left with some high-level comments. Will comment more when there are functionality and API changes coming in.

There are two possible ways to separate the implementation, I'm currently considering v1 here. The current version you proposed is a mixture of v1 and v2. After v1 is done we can then try to move some of the adapting codes from UnicodePlots and ImageInTerminal together and create a new adapter package (v2) but I think that's the future target so in this PR it's better to make things clear and stick to the simpler v1 version. I use the name AsciiPixel here as I feel the name ImageEncoder is a little bit ambiguous; it can mean standard image formats, e.g., PNG/JPEG/TIFF encoder.

image

image

Mostly okay but I hope we can revisit the encodeimg+imshow design a little bit more and try to solve the performance issue; to make a better backend. The current implementation has one performance bottleneck that you have to encode the entire images first before printing them to the IO; from the encoding pipeline it's better to fuse them together into a row-wise for loop, e.g.,

# could further improve the performance by reusing the buffers
@sync for row in img_rows
    buffer = encodeimg(row)
    @async print(io, buffer)
end

Can you remove the feature changes #49 and #57 in this PR? It's unrelated to this PR so it's easier to review it after this PR is merged; fewer things to discuss. I hope we can slow down the refactoring process a bit and put more effort into making a better and cleaner backend in this PR.

ImageEncoding/src/ImageEncoding.jl Outdated Show resolved Hide resolved
ImageEncoding/Project.toml Outdated Show resolved Hide resolved
src/imshow.jl Outdated Show resolved Hide resolved
@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 21, 2021

Can you remove the feature changes #49 and #57 in this PR? It's unrelated to this PR so it's easier to review it after this PR is merged; fewer things to discuss. I hope we can slow down the refactoring process a bit and put more effort into making a better and cleaner backend in this PR.

I've removed the #49 part to stay on v1 scheme.
ImageEncoder has been renamed to AsciiPixel

AsciiPixel/Project.toml Outdated Show resolved Hide resolved
src/imshow.jl Outdated Show resolved Hide resolved
@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 22, 2021

  • Rename ___256 to ___8bit for consistency with the ___24bit color depth.
  • Simplified the tests structure.
  • imshow now dispatches to backend (sixel_encode or ascii_encode) or recursive imshow if img is not an AbstractVecOrMat.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me. It's a good start but there are still a few things I'm concerned with:

  • the API of ancii_encode is still a little bit chaotic to me; it's a mixture of the new usage and the old usage. We might need to figure out a better and cleaner way, decide which part belongs to internal implementation and which part belongs to the public interface. And then rewrite the docstrings a little bit. If you're going to design a backend that faithfully encodes the image into ASCII format and also allows potential "faithful" decoding from ASCII format, what should be kept and what should be removed?
  • Should AsciiPixel be Julia >= 1.6 or Julia >= 1.0? If it requires Julia at least 1.6 then UnicodePlots have to bump to 1.6 as well.
  • This PR contains a lot of changes that can be independently submitted, so we can't just squash and merge it. A direct merge commit would also pollute the git history and make things intractable. We need to either rebase it carefully or split it into multiple PRs. I could do a manual check-in next weekend if rebasing turns out unfamiliar to you.
  • deprecations for types and functions are not handled in this PR; we should include them before considering merging this PR.

As general advice for cleaner and trackable git history; it's recommended to make style changes only to lines where you make functionality changes around. For example, changes like

- RGB(0.5,0.1,0.9)
+ RGB(.5, .1, .9)

are actually unnecessary.

src/ImageInTerminal.jl Outdated Show resolved Hide resolved
AsciiPixel/src/ascii_encode.jl Outdated Show resolved Hide resolved
AsciiPixel/src/AsciiPixel.jl Outdated Show resolved Hide resolved
AsciiPixel/src/AsciiPixel.jl Outdated Show resolved Hide resolved
@johnnychen94
Copy link
Member

johnnychen94 commented Dec 22, 2021

I'm sorry that I'm still busy with the school stuff so can't give a very clear and concrete target here; if the remaining targets of this PR turn out untrackable for you I can continue on top of your work next weekend.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 22, 2021

the API of ancii_encode is still a little bit chaotic to me; it's a mixture of the new usage and the old usage. We might need to figure out a better and cleaner way, decide which part belongs to internal implementation and which part belongs to the public interface. And then rewrite the docstrings a little bit. If you're going to design a backend that faithfully encodes the image into ASCII format and also allows potential "faithful" decoding from ASCII format, what should be kept and what should be removed?

👍

Should AsciiPixel be Julia >= 1.6 or Julia >= 1.0? If it requires Julia at least 1.6 then UnicodePlots have to bump to 1.6 as well.

Let's make it simple and make it 1.6, we'll bump UnicodePlots as well. Current version of UnicodePlots is stable and robust enough, we're adding new things here.

This PR contains a lot of changes that can be independently submitted, so we can't just squash and merge it. A direct merge commit would also pollute the git history and make things intractable. We need to either rebase it carefully or split it into multiple PRs. I could do a manual check-in next weekend if rebasing turns out unfamiliar to you.

Well, I'd prefer to finish the whole stuff now and I'll rebase later when everything will be correctly implemented.

I'm sorry that I'm still busy with the school stuff so can't give a very clear and concrete target here; if the remaining targets of this PR turn out untrackable for you I can continue on top of your work next weekend.

No problem, your comments helped me a lot, I'm not blocked by the code structure right now.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 22, 2021

I hope we can revisit the encodeimg+imshow design a little bit more and try to solve the performance issue; to make a better backend. The current implementation has one performance bottleneck that you have to encode the entire images first before printing them to the IO; from the encoding pipeline it's better to fuse them together into a row-wise for loop

@johnnychen94, we can now use ascii_display in imshow to directly write to the output stream, without using an intermediate buffer, so I think this solves the performance issue.

ascii_encode now only encodes to io (it's cleaner), whereas the downsizing is handled in downscale_big or downscale_small.

@t-bltg t-bltg force-pushed the design branch 5 times, most recently from f64ef90 to fc04886 Compare December 23, 2021 11:25
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