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

support alpha argument in band() #3916

Merged
merged 4 commits into from
Aug 8, 2024
Merged

Conversation

aplavin
Copy link
Contributor

@aplavin aplavin commented May 30, 2024

Description

The alpha argument was just silently ignored before this PR. This makes it actually work.

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@aplavin
Copy link
Contributor Author

aplavin commented Jun 11, 2024

bump :)

@jkrumbiegel
Copy link
Member

can't mesh receive the alpha attribute itself, forwarded?

@aplavin
Copy link
Contributor Author

aplavin commented Jun 11, 2024

Not sure how it all works together... From the code around, seems like all attrs are already forwarded to mesh, but specifying alpha for band plots doesn't work for now.

@jkrumbiegel
Copy link
Member

@SimonDanisch is mesh expected to support alpha?

@aplavin
Copy link
Contributor Author

aplavin commented Jun 26, 2024

bump...
band with alpha would be convenient

@SimonDanisch
Copy link
Member

This is a CairoMakie issue, so I don't think this is the correct fix.
GLMakie works fine:

using GLMakie
xs = 1:0.2:10
ys_low = -0.2 .* sin.(xs) .- 0.25
ys_high = 0.2 .* sin.(xs) .+ 0.25

band(xs, ys_low, ys_high, alpha=0.5)

image
I'm guessing the problem is in https://github.com/MakieOrg/Makie.jl/blob/master/CairoMakie/src/overrides.jl?

@jkrumbiegel
Copy link
Member

A right of course, it's just the override that ignores it.

@aplavin
Copy link
Contributor Author

aplavin commented Jul 16, 2024

Should be fixed properly now! Local testing with CairoMakie shows band transparency.

@aplavin
Copy link
Contributor Author

aplavin commented Jul 30, 2024

bump...
closes #3356

@SimonDanisch
Copy link
Member

Sorry for the long wait, these overrides are a bit hacky and not that great to review for me, since it's not quite clear to me what the "correct" way of doing things is here.
I think we should just merge this, and clean up the overrides once we support having backends decide which plots are "atomic".
@jkrumbiegel does that sound good?

@SimonDanisch SimonDanisch merged commit cb2ae79 into MakieOrg:master Aug 8, 2024
17 of 18 checks passed
@SimonDanisch
Copy link
Member

Thank you!

@aplavin aplavin deleted the band branch August 24, 2024 15:10
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.

3 participants