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

add logical expressions for ispressed #1222

Closed
wants to merge 13 commits into from
Closed

Conversation

ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Aug 15, 2021

Adds the possibility to pass logical expressions to ispressed, for example

scene = Scene()

trigger = Node(nothing)
on(x -> trigger[] = nothing, events(scene).keyboardbutton)
on(x -> trigger[] = nothing, events(scene).mousebutton)

hotkey = Node(Makie.@logical Keyboard.left_shift && !Keyboard.left_control && Mouse.left)
on(trigger) do _
    println(ispressed(scene, hotkey[]))
end
scene

where hotkey lowers to And(Keyboard.left_shift, And(Not(Keyboard.left_control), Mouse.left)). So you could set up key combinations (a && b), alternative hotkeys (a || b) and everything in between. Performance is the same if not a bit better (BenchmarkTools is given me less frequent and lower max times for the case above).

With these changes things like
https://github.com/JuliaPlots/Makie.jl/blob/194e767e207b8b67796412b1fe40fd7fd18f54c6/src/camera/camera3d.jl#L275
could be simplified to if dragging[] && ispressed(scene, button[]).

I also added a ispressed(scene, ::Bool) so you can have a hotkey be always active (true) or always inactive (false). This would add a way to disable an interaction that relies on ispressed.

@SimonDanisch
Copy link
Member

Hm, I guess that's a nice idea!
I think we could also overload & | and ! instead of using a macro:

struct And
 a::Keyboard.Button
 b::Keyboard.Button
end
Base.:(&)(a::Keyboard.Button, b::Keyboard.Button) = And(a, b)
julia> Keyboard.left_alt & Keyboard.left_shift
And(Makie.Keyboard.left_alt, Makie.Keyboard.left_shift)

@ffreyer
Copy link
Collaborator Author

ffreyer commented Aug 26, 2021

I'm thinking about adding something like only(button_expression) or exactly(button_expression) so you can have soft and hard button combinations. I.e. have ispressed(scene, Keyboard.a) return true with (a, b) pressed but ispressed(scene, exactly(Keyboard.a)) return false unless it's just (a,). The question is how this should work between mouse and keyboard buttons - should exactly(Keyboard.a) mean no mouse buttons?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Aug 29, 2021

Changes:

  • removed macro in favor of & | and ! overloads
  • added Exclusively to do hard matches as described above
  • updated cam2d! and cam3d! to use ispressed. So now you can disable interactions via the keys you pass and you can modify trigger them with complex combinations from the outside
  • added tests for ispressed
  • added docs for ispressed

@ffreyer ffreyer marked this pull request as ready for review August 29, 2021 14:30
@SimonDanisch
Copy link
Member

Awesome! :) This is ready to go right?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Sep 29, 2021

I did a quick merge to see if tests fail. I think the docs I wrote might be broken now that we moved to Franklin.

I also had a couple of concerns that I didn't mention yet.

I'm wondering if it would be good to have variants of ispressed to detect when a button or combination has just become active or has just become inactive. Because currently you have to go back to backend events to deal with that. I played around with a waspressed(scene, keys, removed_key) function but I can't find that code anymore.

Since ispressed works with mouse and keyboard buttons I'm wondering whether we should add an onyany that works with just PriorityObservables. You'd generally want to trigger on both and onany would simplify that.

When I thought about this last I decided that I didn't want to make this pr more complicated. Maybe it's better to leave this as is and make another pr later adding that stuff?

@SimonDanisch
Copy link
Member

When I thought about this last I decided that I didn't want to make this pr more complicated

Yeah, I think that's a good idea!

SimonDanisch added a commit that referenced this pull request Oct 16, 2021
@SimonDanisch SimonDanisch mentioned this pull request Oct 19, 2021
@SimonDanisch
Copy link
Member

Merged in #1393

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