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

Extend color for tuple of RGB components #145

Merged
merged 11 commits into from
Aug 19, 2021
Merged

Conversation

t-bltg
Copy link
Member

@t-bltg t-bltg commented Aug 18, 2021

Allow passing arbitrary colors, e.g.:

using UnicodePlots

lineplot([1, 2], color=:black)
lineplot([1, 2], color=(0, 100, 200))
lineplot([1, 2], color=255)

Needed for Plots.jl, since we use RGB values internally.
Uses Crayon for color conversion: I think this is appropriate, since Crayon is already a UnicodePlots dependency used in Heatmaps, and its ANSIColor type is coherent with terminal color output.

With #136 applied, all tests pass with newly generated images:

Test Summary: | Pass  Total
tst_common.jl |  106    106
Test Summary:   | Pass  Total
tst_graphics.jl |   79     79
Test Summary: | Pass  Total
tst_canvas.jl |  140    140
Test Summary: | Pass  Total
tst_plot.jl   |   64     64
Test Summary:  | Pass  Total
tst_barplot.jl |   32     32
Test Summary:    | Pass  Total
tst_histogram.jl |   19     19
Test Summary:      | Pass  Total
tst_scatterplot.jl |   45     45
Test Summary:   | Pass  Total
tst_lineplot.jl |   79     79
Test Summary: | Pass  Total
tst_spy.jl    |   16     16
Test Summary:  | Pass  Total
tst_boxplot.jl |   20     20
Test Summary:  | Pass  Total
tst_heatmap.jl |   75     75

Fix #20.
Fix #58.

col = color in keys(color_decode) ? color_decode[color] : Int(color)
str = string(args...)
printstyled(io, str; color = col)
function crayon_256_color(color::UserColorType)::ColorType
Copy link
Member Author

@t-bltg t-bltg Aug 18, 2021

Choose a reason for hiding this comment

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

@johnnychen94, this seems a bit hacky, should I PR to Crayons.jl instead to have stable API conversions to 256 color ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Proposing a stable API in Crayons sounds okay to me but I know very little about this. I don't know if it's useful, there are some discussions on JuliaGraphics/Colors.jl#473

Copy link
Member Author

@t-bltg t-bltg Aug 18, 2021

Choose a reason for hiding this comment

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

Thanks for the link. What we need in UnicodePlots is something generic working on all terminals. Since we use printstyled(...) I think the implementation in this PR is ok using 0-255 integers.

@KristofferC, do you have any advice for this ?

@t-bltg
Copy link
Member Author

t-bltg commented Aug 18, 2021

The CI failures are related to base/util.jl inserting 38;5; to set the foreground color, when using integers in the range 0-255 even if visually the images are correct. I must thus regenerate reference images.

Except for Heatmap, for which this PR fixes a regression, dotcanvas & heatmapcanvas are now similar in terms of colors for the same data:
img1

img2

Copy link
Collaborator

@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.

Sorry for the back and forth. I just noticed that this PR doesn't have associated test cases added. Can you also add some tests for the new keyword types?

Currently, when I try barplot(["B", "A"], [2, 1]; color=(255, 0, 0)) it's broken. Looks like something is missing here?

src/common.jl Outdated Show resolved Hide resolved
Co-authored-by: Johnny Chen <[email protected]>
@t-bltg
Copy link
Member Author

t-bltg commented Aug 19, 2021

I just noticed that this PR doesn't have associated test cases added

Good point, will do.

Can you also add some tests for the new keyword types?

I don't understand, could you precise which keyword types you're talking about ?

Currently, when I try barplot(["B", "A"], [2, 1]; color=(255, 0, 0)) it's broken.

Good catch ! Will fix.

@johnnychen94
Copy link
Collaborator

Can you also add some tests for the new keyword types?

I don't understand, could you precise which keyword types you're talking about ?

Sorry for the inaccuracy, I meant to test NTuple{3, Integer} types for color keyword.

@t-bltg
Copy link
Member Author

t-bltg commented Aug 19, 2021

Sorry for the inaccuracy, I meant to test NTuple{3, Integer} types for color keyword.

Oh ok, yes 👍

@t-bltg t-bltg force-pushed the rgb branch 3 times, most recently from 9145af8 to c1d05eb Compare August 19, 2021 10:36
@t-bltg t-bltg force-pushed the rgb branch 4 times, most recently from 1cb087e to 5b83026 Compare August 19, 2021 10:49
@t-bltg t-bltg marked this pull request as draft August 19, 2021 10:52
@t-bltg t-bltg force-pushed the rgb branch 6 times, most recently from a6e07f9 to f126e20 Compare August 19, 2021 12:12
@t-bltg
Copy link
Member Author

t-bltg commented Aug 19, 2021

I've inserted some colors tests covering NTuple{3, Integer} and Integer in tst_canvas, tst_barplot, tst_boxplot, tst_histogram.

We now have 3 colors type: UserColorType (broad), ColorType for UnicodePlots canvas, and finally JuliaColorType for printstyled.

After visual inspection against master using:

root="$HOME/Downloads/UnicodePlots.jl"  # fork

for f in $(find $root/test/references -name '*.txt'); do
 echo; echo
 echo "== local($f) =="
 cat $f
 echo
 url="https://raw.githubusercontent.com/Evizero/UnicodePlots.jl/master/${f#$root/}"
 echo "== remote($url) =="
 wget -q -O - $url
done

, I think this PR can be reviewed again.

@t-bltg t-bltg marked this pull request as ready for review August 19, 2021 12:30
@t-bltg t-bltg requested a review from johnnychen94 August 19, 2021 12:30
Copy link
Collaborator

@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.

Sorry, I didn't look very carefully but the test looks good to me. Since a lot of references are changed so I assume there will be some implicit behavior differences, I'd like to bump a minor version to indicate some "breaking" changes, does it sound good to you? @t-bltg

@t-bltg
Copy link
Member Author

t-bltg commented Aug 19, 2021

Sorry, I didn't look very carefully but the test looks good to me. Since a lot of references are changed so I assume there will be some implicit behavior differences, I'd like to bump a minor version to indicate some "breaking" changes, does it sound good to you? @t-bltg

Yes, I'm good with the minor bump. I will apply the changes in Plots.jl accordingly after the release.

@johnnychen94 johnnychen94 changed the title Allow passing color = symbol, integer or tuple of RGB values Extend color for tuple of RGB components Aug 19, 2021
@johnnychen94 johnnychen94 merged commit 8edd8f9 into JuliaPlots:master Aug 19, 2021
@t-bltg t-bltg deleted the rgb branch August 19, 2021 14:50
@johnnychen94
Copy link
Collaborator

It's actually a major version bump 😄 v2.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for 256 colors. Support Colors.Colorant
2 participants