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

Custom interactions #4531

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

EdsterG
Copy link
Contributor

@EdsterG EdsterG commented Oct 25, 2024

Description

Partially Fixes #3950

The goal of this PR is to allow users to change default interactions, by modifying a global registry.

  • Define a global registry of default interactions for Axis and Axis3D.
  • Update RectangleZoom to use the registration functions registration_setup! and deregistration_cleanup!.
  • Update RectangleZoom deactivate when modifier key (if one is defined) is released before the mouse.
  • Allow LimitReset to be customizable.

I understand that a small subset of the interactions can be changed as an argument to Axis, however that doesn't solve the general problem of changing the default values for those interactions. For users that prefer to use non-default values, instead of forcing them to constantly pass a bunch of extra kwargs to Axis, this PR will allow them to specify new defaults, one time.

Type of change

Delete options that do not apply:

  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in CHANGELOG.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.

@EdsterG EdsterG marked this pull request as draft October 25, 2024 19:41
@EdsterG
Copy link
Contributor Author

EdsterG commented Oct 30, 2024

Any high level thoughts on this @ffreyer?

@EdsterG
Copy link
Contributor Author

EdsterG commented Dec 7, 2024

@SimonDanisch I think Makie is very close to becoming the goto library for interactive dashboard. In my opinion, allowing users to customize interactions will help solidify Makie as the best library for complex dashboard. Here's a simple use case which requires the default interactions to change. Right click to drag/pan, left click to control the vertical bar, modifier + left click to box zoom.

example.mov

@EdsterG EdsterG marked this pull request as ready for review December 9, 2024 17:29
@SimonDanisch
Copy link
Member

Thanks!
I think this should be part of the theme, shouldn't it?

@EdsterG
Copy link
Contributor Author

EdsterG commented Dec 10, 2024

I think interactions can go both ways, part of the theme or not part of the theme. They aren't currently part of theme. What's your preference? If you prefer having them be part of the theme, how do you suggest they be added? Doing something like this Axis3 = (interactions=Dict(...),) feels a bit unnatural and clunky because it's bypassing the register/deregister interface.

# Conflicts:
#	CHANGELOG.md
#	src/makielayout/blocks/axis3d.jl
@EdsterG
Copy link
Contributor Author

EdsterG commented Dec 23, 2024

@SimonDanisch any suggestions for how to make this part of the theme? Or shall it be left as is for now?

@@ -162,8 +174,8 @@ mutable struct RectangleZoom
modifier::Any # e.g. Keyboard.left_alt, or some other button that needs to be pressed to start rectangle... Defaults to `true`, which means no modifier needed
end

function RectangleZoom(callback::Function; restrict_x=false, restrict_y=false, modifier=true)
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the callback? It's there to make custom RectangleZoom interactions...
Should be better documented, I suppose...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed the interactions had the following unused interface:

function registration_setup!(parent, interaction)
    # do nothing in the default case
end

function deregistration_cleanup!(parent, interaction)
    # do nothing in the default case
end

So I updated RectangleZoom to use that interface, it makes dealing with interactions much easier. The callback seemed like a round-about thing.

The callback used to be defined here:

function RectangleZoom(ax::Axis; kw...)
    return RectangleZoom(ax; kw...) do newlims
        if !(0 in widths(newlims))
            ax.targetlimits[] = newlims
        end
        return
    end

and called inside process_interaction:

    elseif event.type === MouseEventTypes.leftdragstop
        try
            r.callback(r.rectnode[])
        catch e
            @warn "error in rectangle zoom" exception=(e, Base.catch_backtrace())
        end
        r.active[] = false
        return Consume(true)

From what I can tell it was just passing a reference to ax in a round-about way. This pattern is also error prone because there's some ax referenced at construction and another in process_interaction. It doesn't make sense for them to be different axes.

Now it just modifies ax inside process_interaction directly:

    elseif event.type === MouseEventTypes.leftdragstop
        newlims = r.rectnode[]
        if !(0 in widths(newlims))
            ax.targetlimits[] = newlims
        end
        r.active[] = false
        return Consume(true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what it's worth, rectangle zoom still works with the change in this PR.

Copy link
Contributor Author

@EdsterG EdsterG Jan 13, 2025

Choose a reason for hiding this comment

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

Would you point me to a use case where a custom callback is required for RectangleZoom? Prior to this PR there was no way for users to customize RectangleZoom without modifying the source code (unless I'm missing something).

Copy link
Member

Choose a reason for hiding this comment

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

I introduced this to be able to register your own interaction for rectangle zoom:

f, ax, pl = scatter(1:4)
deregister_interaction!(ax, :rectanglezoom)
zoom = Makie.RectangleZoom(ax) do rect
    @show rect # do something with rectangle
end
register_interaction!(ax, :rectanglezoom, zoom)

I admit it's a bit awkward how it uses the interface.
Maybe we can keep how you refactored it and figure out a way how to get the best of both worlds.
Maybe an optional user callback, and otherwise it uses the axis from the setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can add an optional user callback. Though I would prefer the callback to take an ax argument for the reason mentioned above, if you're ok with that.

f, ax, pl = scatter(1:4)
deregister_interaction!(ax, :rectanglezoom)
zoom = Makie.RectangleZoom(ax) do ax, rect
    @show rect # do something with rectangle
end
register_interaction!(ax, :rectanglezoom, zoom)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, with this PR it'll be super easy for something to create RectangleZoomWithSpecialFeature and make that the new default behavior (or just register it to a specific plot as in your example).

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

Successfully merging this pull request may close these issues.

FR: Add ability to change the "reset zoom" binding
2 participants