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

command: make script-binding command scalable #15316

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

Conversation

na-na-hi
Copy link
Contributor

This makes script-binding command and key bindings added by mp.add_key_binding() scalable. The script-message command now has an extra argument with the scale of the key.

Copy link

github-actions bot commented Nov 14, 2024

Download the artifacts for this pull request:

Windows
macOS

@avih
Copy link
Member

avih commented Nov 15, 2024

The script-message command now has an extra argument

Did you mean script-binding? I don't think I see at the changes anything specific to script-message, but I do see changes to script-binding.

If it's still script-message, then at which level is the extra argument added?

The script-message command itself - as clients use it, takes any number of arbitrarry arguments, and by convention the first is the message name.

So adding an argument at this level will surely break existing clients which expect the receiver to see exactly the arguments they send.

Or maybe it's only in how script-binding uses script-message?

Please clarify.

@na-na-hi
Copy link
Contributor Author

na-na-hi commented Nov 15, 2024

Did you mean script-binding? I don't think I see at the changes anything specific to script-message, but I do see changes to script-binding.

It's noted in the changed script-binding documentation. Internally script-binding calls script-message command with several arguments. This makes script-binding calling script-message with an additional argument of scale.

I have updated the commit message to clarify this.

So adding an argument at this level will surely break existing clients which expect the receiver to see exactly the arguments they send.
Or maybe it's only in how script-binding uses script-message?

This only affects how script-binding uses script-message. The script-binding documentation explicitly notes: "Future versions can add more arguments and more key state characters to support more input peculiarities.", so it doesn't guarantee stability, but for this PR the extra argument is added to the end to avoid breaking existing clients.

script-binding command is currently not scalable, so script
registered key bindings also cannot be scalable, unlink input.conf
bindings.

This makes script-binding command scalable so that it's possible to
define scalable key bindings in scripts. It now calls script-message
command with an extra argument with the scale of the key.
For complex key bindings, the table now contains a new member
of the current key scale. mp.add_key_binding() now accepts the
scalable flag to make the binding scalable.
This documents the commands which are scalable, and refer
to information on how it affects input processing.
@na-na-hi
Copy link
Contributor Author

Added some documentation for scalable keys and scalable commands to better clarify scalable stuffs as a whole.

This documents the scalable keys (currently WHEEL_*) and notes
how the keys work with scalable commands. Mention touch pad
as a common source of scalable input source.
@guidocella
Copy link
Contributor

guidocella commented Nov 15, 2024

We can't actually use this for cursor-centric-zoom can we? Because that needs to be bound to script-message in input.conf and not script-binding to send the zoom argument.

@na-na-hi
Copy link
Contributor Author

You can add a script key binding for ctrl+wheel_up with mp.add_key_binding() in your script, which will provide the scale information. You can then send a script-message to cursor-centric-zoom with this information, or do the zoom directly if in the same script.

@guidocella
Copy link
Contributor

I mean the user can no longer configure the zoom amount in input.conf. I guess we need to add a script-opt for it, which is more indirect.

@guidocella
Copy link
Contributor

Actually, osd-relative-pan <axis> <amount> which needs to be a script message will have the same issue. Will users that want keybindings to pan with different amounts have to define bindings like

LEFT  {image} no-osd change-list script-opts append positioning-pan_amount=-.2; script-binding positioning/osd-relative-pan-x
DOWN  {image} no-osd change-list script-opts append positioning-pan_amount=+.2; script-binding positioning/osd-relative-pan-y
UP    {image} no-osd change-list script-opts append positioning-pan_amount=-.2; script-binding positioning/osd-relative-pan-y
RIGHT {image} no-osd change-list script-opts append positioning-pan_amount=+.2; script-binding positioning/osd-relative-pan-x
Shift+LEFT  {image} no-osd change-list script-opts append positioning-pan_amount=-.02; script-binding positioning/osd-relative-pan-x
Shift+DOWN  {image} no-osd change-list script-opts append positioning-pan_amount=+.02; script-binding positioning/osd-relative-pan-y
Shift+UP    {image} no-osd change-list script-opts append positioning-pan_amount=-.02; script-binding positioning/osd-relative-pan-y
Shift+RIGHT {image} no-osd change-list script-opts append positioning-pan_amount=+.02; script-binding positioning/osd-relative-pan-x

On top of no-osd not working these are multiple hoops users have to jump through to not make these commands.

@na-na-hi
Copy link
Contributor Author

Actually, osd-relative-pan <axis> <amount> which needs to be a script message will have the same issue. Will users that want keybindings to pan with different amounts have to define bindings like

The case you mentioned can simply be a script-message with scrolling amount as a parameter. script-binding is only needed to get high resolution scrolling, so I think it's OK to keep it a separate option from the rest. The script can have both the script-message and script-binding to cover both cases.

@guidocella
Copy link
Contributor

In my opinion having both script-message and script-binding for the same function is more complex to write and document than to just write it in C and extremely confusing for users, how are they supposed to understand which one to use? And users that want both high resolution scaling and bindings with different amounts still have to define user-unfriendly bindings like above.
I think we can implement osd relative pan and cursor centric zoom as commands, because commands can both accept arbitrary arguments and factor high resolution scaling. This lets users configure pan and zoom amounts in input.conf like with properties.
Cursor centric zoom is already implemented in C and osd relative pan is trivial.
As a bonus, cursor centric zoom will respect the standard no-osd prefix instead of configuring it indirectly with a script-opt.
Then keep drag to pan and align to cursor as a script, because they are too hard to reimplement in C since they temporarily rebind MOUSE_MOVE/observe mouse-pos, and there is no advantage in making them commands since they don't accept arguments.
We could then name the script misc.lua and reuse it for future key bindings, like #13837 (comment)

@na-na-hi
Copy link
Contributor Author

In my opinion having both script-message and script-binding for the same function is more complex to write and document than to just write it in C and extremely confusing for users, how are they supposed to understand which one to use? And users that want both high resolution scaling and bindings with different amounts still have to define user-unfriendly bindings like above.

The intended way is to configure keybinds and amounts as script options, and let the script create and destroy key bindings by itself, and doesn't require putting anything into input.conf. input.conf isn't suitable for anything complex or specialized, and it's more confusing to use compared to making them all available as script options.

The only reason they are useful as script messages is for other scripts to call them. I don't think they're needed on their own when script options serve the purpose.

I think we can implement osd relative pan and cursor centric zoom as commands, because commands can both accept arbitrary arguments and factor high resolution scaling. This lets users configure pan and zoom amounts in input.conf like with properties.

Like I said script options is a better choice.

Cursor centric zoom is already implemented in C and osd relative pan is trivial.

I mentioned in your PR how features like animated zooming would be much harder to do in C while being easier in lua. mpv clients are clearly in a better place to implement these features compared in player core.

Then keep drag to pan and align to cursor as a script, because they are too hard to reimplement in C since they temporarily rebind MOUSE_MOVE/observe mouse-pos, and there is no advantage in making them commands since they don't accept arguments.

Yeah, some are commands and others are script messages... totally not confusing which you seem trying so hard to avoid.

I looked into making the script-message scalable but decided against it because it makes little sense when all arguments are strings, but I would rather hack scaling into it (when the first argument looks like a number) if you still insist on the script-message approach than whatever you're suggesting here.

@guidocella
Copy link
Contributor

The intended way is to configure keybinds and amounts as script options, and let the script create and destroy key bindings by itself, and doesn't require putting anything into input.conf. input.conf isn't suitable for anything complex or specialized, and it's more confusing to use compared to making them all available as script options.

I don't think zoom and pan amounts are complex or specialized. All key bindings that modify properties already configure amounts in input.conf, it is nothing new.

I mentioned in your PR how features like animated zooming would be much harder to do in C while being easier in lua. mpv clients are clearly in a better place to implement these features compared in player core.

We can add a command for osd-relative-pan and keep cursor-centric-zoom in a script a middle ground. Wanting key bindings that pan with different amounts should be much more common than key bindings that zoom in different amounts. This is also the case in https://github.com/occivink/mpv-image-viewer/blob/master/input.conf, it has multiple pan amounts and one cursor-centric-zoom amount. I don't know if having to define the bindings in #15316 (comment) just do that seems sane to you. ¯\(ツ)/¯ But if maintainers deem it fine I'm fine with either way.

Yeah, some are commands and others are script messages... totally not confusing which you seem trying so hard to avoid.

Most mpv key bindings are already commands and some script messages, the only difference is that I'm adding these ones at the same time.

I looked into making the script-message scalable but decided against it because it makes little sense when all arguments are strings, but I would rather hack scaling into it (when the first argument looks like a number) if you still insist on the script-message approach than whatever you're suggesting here.

Nah, that is too hacky. You can't know whether numbers for script-message are floats or integers not meant to be scaled. Also ideally the axis would ideally be the first argument to pan.

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