Only apply axis/scene transformations to :data space plots #2436

dgleich · 2022-11-22T22:04:12Z

Description

Fixes #2424 Fixes #2065

This PR adjusts the behavior of the axis/scene transformations to only apply to plots in the :data space.

Notes

Examined and editing almost all places where "apply_transform" was used to switch to the new three parameter version suggested in #2065.

There were some places where the existing behavior was left. This is due to a quick survey of the surrounding logic suggesting that the existing code assumes things are transformed. Places where this happens:

NO CHANGE src/basic_recipes/hvspan.jl:_apply_x_transform(t::Tuple, v) = apply_transform(t[1], v)
NO CHANGE src/basic_recipes/hvspan.jl:_apply_y_transform(t::Tuple, v) = apply_transform(t[2], v)
NO CHANGE src/layouting/data_limits.jl: point_t = apply_transform(trans_func, point)
NO CHANGE src/layouting/data_limits.jl: (to_ndim(Point3f, project(model, apply_transform(trans_func, point)), 0f0) for point in points)
NO CHANGE src/makielayout/blocks/axis.jl: tlims = Makie.apply_transform(t, lims)

Type of change

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

Checklist

Added an entry in NEWS.md (for new features and breaking changes)
Added or changed relevant sections in the documentation
Added unit tests for new algorithms, conversion methods, etc.
Added reference image tests for new plotting functions, recipes, visual options, etc.

ffreyer · 2022-11-23T00:40:41Z

src/layouting/transformation.jl

+Apply the data transform func to the data if the space matches one
+of the the transformation spaces (currently only :data is transformed)
+"""
+apply_transform(f, data, space) = space == :data ? apply_transform(f, data) : data  


Just to point it out - this might need to be copy(data) if there are any mutating operations done on the output. I don't think there are, since we generally go through observables which create copies, but I thought I'd mention it anyway.

The identity transforms make a point of returning data (or x) directly (and verifies that by checking === in one of the test cases), so that behavior was mirrored here. I would think any issues with mutating the data would quickly show up in that regard. Let me know if you think this needs any changes.

Yeah, I did that at the time because I thought the overhead of copying was unnecessary. And because I didn't see the need for mutation afterwards, but still one should be careful

Is there a change (documentation note maybe?) or test you would like to insert here to make sure this stays consistent in the future?

src/makielayout/interactions.jl

dgleich · 2022-11-23T03:31:23Z

I spotted a few other places in GLMakie.jl where I wasn't wrapping the space observable in a map. (When I was testing locally, this initially caused the log10 heatmap reference image to fail before I fixed it.) The other cases are in function mesh_inner(screen::Screen, mesh, transfunc, gl_attributes) and function draw_atomic(screen::Screen, scene::Scene, x::Surface) which I'm guessing don't have tests with log10 scaled axis. I've edited them locally and am just running tests again before updating the PR. Will fix that and update shortly

dgleich · 2022-11-23T13:46:36Z

Okay, I fixed the other cases... GLMakie.jl passes tests locally.

SimonDanisch · 2022-11-25T12:50:41Z

Should we make this non breaking by having apply_transform(f, data, space = :data) ?

jkrumbiegel · 2022-11-28T09:13:55Z

Is apply_transform exported?

SimonDanisch · 2022-11-28T09:24:33Z

It's used in other packages, e.g. GeoMakie 🤷

jkrumbiegel · 2022-11-28T09:59:17Z

Hm ok maybe it's the path of least resistance then?

SimonDanisch · 2022-11-28T10:34:57Z

Yeah would be nice to not break GeoMakie if it's so easy to avoid...

ffreyer · 2022-11-28T11:07:55Z

This shouldn't break anything. All the other spaces should exclude transformation function by definition. space = :pixel means a 1:1 corresponds between (x, y) inputs and pixel positions. space = :clip is strictly a [-1, 1] x [-1, 1] x [0, 1] coordinate system. space = :relative is a [0, 1] x [0, 1] x [0, 1] coordinate system. Having a transformation function other than identity will break these conventions.

SimonDanisch · 2022-11-28T11:19:34Z

The breaking change is that apply_transform needs 3 arguments now, as far as I can tell...

ffreyer · 2022-11-28T13:00:28Z

apply_transform(f, data, space) = space == :data ? apply_transform(f, data) : data

Both exist. For implementing new functions you'd still extend the 2 argument version, but for usage it should generally be the 3 argument version.

SimonDanisch · 2022-11-28T13:40:22Z

Jeez, you're right... I really need to up my github diff reading game ;)
GeoMakie passes tests on this branch :)

@dgleich, could you add a minimal test plot to: https://github.com/MakieOrg/Makie.jl/blob/master/ReferenceTests/src/tests/short_tests.jl#L247, which now works with your changes?

Just add a block like:

@reference_test "array of rects poly" begin
    plot_that_looks_correct_now(...)
end

dgleich · 2022-11-28T14:56:43Z

@SimonDanisch Added! Was going to inquire if you wanted something like that too :)

SimonDanisch · 2022-11-28T16:23:36Z

This looks wrong, no?

dgleich · 2022-11-28T17:09:51Z

@SimonDanisch That's what I get on the current master. With the updates, it should look like this

SimonDanisch · 2022-11-28T17:12:18Z

This is the picture the CI created from the current run... And if I check out the PR locally, I get the same... How did you produce the picture?

dgleich · 2022-11-28T17:31:36Z

Hmm... the CairoMakie images I downloaded here look okay and ran okay locally...

https://github.com/MakieOrg/Makie.jl/actions/runs/3565833787

It looks like something changed in GLMakie.jl -- let me take a quick look.

dgleich · 2022-11-28T17:53:58Z

Okay, tests are useful :)

The poly command for GLMakie.jl is the one that gives the problem. (This works with lines and scatter...) I'm still debugging.

jkrumbiegel · 2022-11-28T18:28:59Z

In CairoMakie, poly is drawn directly while it is drawn as nested mesh + wireframe in GLMakie

dgleich · 2022-11-28T19:02:47Z

Found the issue -- connect_camera! deletes the :space attribute in gl_attributes

I can work around this. (e.g. the routines that did work pulled out the space attribute before connect_camera.)

Is GLMakie.mesh_inner used in other packages? The easiest way to patch this is to give that the space parameter before connect_camera deletes it. That's what I did in the most recent set of edits I'll push after I double check the test works :)

dgleich · 2022-11-28T19:13:11Z

Okay, new code passes the simple test. I added another test that tries to plot an image into a transformed axis with pixel... it fails on master and works with the new code (yay!)

SimonDanisch · 2022-11-29T13:46:19Z

Ok, this looks good now:

I think we should be able to merge this if CI runs through with the new reference image :)

dgleich · 2022-11-29T13:49:32Z

Looking at fixing it now. I think I had a tiny typo in the new test and I’m being more careful with the final edits.

…

On Tue, Nov 29, 2022 at 08:46 Simon ***@***.***> wrote: Ok, this looks good now: [image: image] <https://user-images.githubusercontent.com/1010467/204545295-cb2f8d5b-2b5d-45e4-95ad-c4253f77309d.png> I think we should be able to merge this if CI runs through with the new reference image :) — Reply to this email directly, view it on GitHub <#2436 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAIJS55K6TFDJMWNFL7A5TWKYCLLANCNFSM6AAAAAASIJQXMA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

dgleich · 2022-11-29T14:22:20Z

Okay, I fixed the typo which passes GLMakie.jl tests locally.

dgleich · 2022-11-29T14:30:48Z

But CairoMakie.jl fails on saving the resulting figure -- I'm just going to remove that extra test with images.

and fix for GLMakie plot

SimonDanisch · 2022-11-29T15:43:03Z

Great :) Thanks for hanging in there and pushing this through! ;)
Sounds like the CairoMakie failure should be turned into an issue at least?

dgleich · 2022-11-29T16:14:30Z

Okay -- I was again too hasty! Turns out the CairoMakie failure is related to this request. Let me see if I can fix that one up too...

The existing code here:

if interpolate
        if !regular_grid
            error("$(typeof(primitive).parameters[1]) with interpolate = true with a non-regular grid is not supported right now.")
        end
        if !identity_transform
            error("$(typeof(primitive).parameters[1]) with interpolate = true with a non-identity transform is not supported right now.")
        end
    end

should also check if we are doing pixel-space plots...

dgleich · 2022-11-29T18:15:35Z

Alright, I think fixing that issue with CairoMakie.jl is going to end up related-to-but-really-disconnected from this issue/pull request. e.g. the current non-fast path doesn't work in CairoMakie.jl so I'm going to open a separate issue to resolve those. e.g. the following code (with no transforms) fails...

fig = Figure()
image(fig[1,1], Makie.logo()', 
    axis=(yreversed=true,),
    fast_path=false)
fig

Does that seem okay to try and merge the current one and defer the other one into a separate issue?

SimonDanisch · 2022-11-29T18:43:32Z

Sounds good to me!

dgleich · 2022-11-30T14:22:46Z

Anything else that I should change on this one? Do I need to re-request a review due to the new changes?

…2436) * Only apply axis/scene transformations to :data space plots * Add test for pixel space plots in transformed axis, and fix for GLMakie plot Co-authored-by: Simon <sdanisch@protonmail.com>

ffreyer reviewed Nov 23, 2022

View reviewed changes

src/makielayout/interactions.jl Outdated Show resolved Hide resolved

dgleich force-pushed the transforms-data-only branch from 78829df to 214ae10 Compare November 23, 2022 02:30

Only apply axis/scene transformations to :data space plots

0bdba06

dgleich force-pushed the transforms-data-only branch from b6994a3 to 0bdba06 Compare November 23, 2022 13:43

Merge branch 'master' into transforms-data-only

46cf570

Merge branch 'master' into transforms-data-only

5d41b47

dgleich force-pushed the transforms-data-only branch from 0e0b330 to c3e0b22 Compare November 28, 2022 14:44

dgleich force-pushed the transforms-data-only branch 2 times, most recently from c6e0aa4 to 1c05c40 Compare November 28, 2022 20:17

dgleich force-pushed the transforms-data-only branch from 1c05c40 to 6fee016 Compare November 28, 2022 20:18

SimonDanisch closed this Nov 29, 2022

SimonDanisch reopened this Nov 29, 2022

dgleich force-pushed the transforms-data-only branch from 6fee016 to 55cdc0f Compare November 29, 2022 14:21

Add test for pixel space plots in transformed axis,

d200c0a

and fix for GLMakie plot

dgleich force-pushed the transforms-data-only branch from 55cdc0f to d200c0a Compare November 29, 2022 14:33

dgleich mentioned this pull request Nov 29, 2022

CairoMakie image with fast_path not working, image with space=:pixel and transformation not working #2457

Closed

SimonDanisch merged commit e77516d into MakieOrg:master Nov 30, 2022

Uh oh!

Only apply axis/scene transformations to :data space plots #2436

Only apply axis/scene transformations to :data space plots #2436

Uh oh!

Conversation

dgleich commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes

Type of change

Checklist

Uh oh!

ffreyer Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

dgleich Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

jkrumbiegel Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgleich Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dgleich commented Nov 23, 2022

Uh oh!

dgleich commented Nov 23, 2022

Uh oh!

SimonDanisch commented Nov 25, 2022

Uh oh!

jkrumbiegel commented Nov 28, 2022

Uh oh!

SimonDanisch commented Nov 28, 2022

Uh oh!

jkrumbiegel commented Nov 28, 2022

Uh oh!

SimonDanisch commented Nov 28, 2022

Uh oh!

ffreyer commented Nov 28, 2022

Uh oh!

SimonDanisch commented Nov 28, 2022

Uh oh!

ffreyer commented Nov 28, 2022

Uh oh!

SimonDanisch commented Nov 28, 2022

Uh oh!

dgleich commented Nov 28, 2022

Uh oh!

SimonDanisch commented Nov 28, 2022

Uh oh!

dgleich commented Nov 28, 2022

Uh oh!

SimonDanisch commented Nov 28, 2022

Uh oh!

dgleich commented Nov 28, 2022

Uh oh!

dgleich commented Nov 28, 2022

Uh oh!

jkrumbiegel commented Nov 28, 2022

Uh oh!

dgleich commented Nov 28, 2022

Uh oh!

dgleich commented Nov 28, 2022

Uh oh!

SimonDanisch commented Nov 29, 2022

Uh oh!

dgleich commented Nov 29, 2022 via email

Uh oh!

dgleich commented Nov 29, 2022

Uh oh!

dgleich commented Nov 29, 2022

Uh oh!

SimonDanisch commented Nov 29, 2022

Uh oh!

dgleich commented Nov 29, 2022

Uh oh!

dgleich commented Nov 29, 2022

Uh oh!

dgleich commented Nov 22, 2022 •

edited

Loading

jkrumbiegel Nov 23, 2022 •

edited

Loading