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

positioning.lua: add this script #15314

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

Conversation

guidocella
Copy link
Contributor

This script provides script messages and bindings to pan videos and images, making mpv a better image viewer out of the box.

I've had this script for 3 years in my image config so why not upstream it to view images better out of the box. Credits to occivink for the original ideas of the bindings.

I also have a mouse gesture and a oneshot double page mode in my script but those may not be worth upstreaming.

For the reasoning behind the suppress_osd naming, see #9051 (comment)

Touch input in cursor-centric-zoom is untested.

For negative zoom, I think we need a global option like --auto-recenter-video (implemented in vo_get_src_dst_rects()?) to reset video-align to 0 when the image is smaller than the OSD, so that it also applies when changing video-zoom/panscan/video-unscaled/video-aspect-override/video-scale/changing file. Then positioning.lua should do nothing when that option is true and the image is smaller than the OSD. In my config, I use

mp.observe_property('osd-dimensions', 'native', function (_, dims)
    if dims.ml + dims.mr > 0 then
        mp.set_property('video-align-x', 0)
    end
    if dims.mt + dims.mb > 0 then
        mp.set_property('video-align-y', 0)
    end
end)

Copy link

github-actions bot commented Nov 14, 2024

Download the artifacts for this pull request:

Windows
macOS

@kasper93
Copy link
Contributor

Can we add default keybindings for those commands? Else it will become dead code.

@guidocella
Copy link
Contributor Author

Sure I was trying not to burden the original proposal with breaking changes. What about:

Move cycle pause to MBTN_LEFT
drag-to-pan on MBTN_RIGHT
align-to-cursor on MBTN_MID
cursor-centric-zoom on Ctrl+wheel
Rebind Alt+Arrows to osd-relative-pan
?

Unfortunately we can't easily set different default key bindings for images if we're set on deprecating input sections.

@mrfragger
Copy link

mrfragger commented Nov 16, 2024

Touch input in cursor-centric-zoom is untested.

I'm using a touchpad but not sure how exactly I'm supposed to keybind it

Ctrl+MBTN_LEFT script-binding drag-to-pan. (this one works)
tried this and invalid command
Ctrl+Alt+WHEEL_UP cursor-centric-zoom
same as this one
Ctrl+Alt+WHEEL_UP cursor-centric-zoom add video-zoom 0.05
Ctrl+UP align-to-cursor (not sure but says invalid command)

@mrfragger
Copy link

thanks for the help..yes the centric-cursor-zoom works great on touchpad. Thanks for adding these...gonna use mpv as main image viewer on mac. These are great additions.

Ctrl+Wheel_Down add video-zoom -0.05 #! [Video/Image] > [Scale] > Zoom Out (scroll down)
Ctrl+Wheel_Up add video-zoom 0.05 #! [Video/Image] > [Scale] > Zoom In (scroll up)
Alt+Wheel_Down script-message cursor-centric-zoom -0.05 #! [Video/Image] > [Scale] > Zoom Out near cursor
Alt+Wheel_Up script-message cursor-centric-zoom 0.05 #! [Video/Image] > [Scale] > Zoom In near cursor
Alt+MBTN_LEFT script-binding align-to-cursor #! [Video/Image] > [Scale] > Pan aligned to cursor (while zoomed in)
Ctrl+MBTN_LEFT script-binding drag-to-pan #! [Video/Image] > [Scale] > Drag to Pan (while zoomed in)

@guidocella
Copy link
Contributor Author

guidocella commented Nov 16, 2024

Yeah I know it works on a touchpad, by untested touch input I meant devices like tables which populate the touch-pos property (or at least I assume touch-pos is not supposed to work with touchpads).

EDIT: I was trying a touchpad on Xorg whose backend doesn't implement touch-pos, it should work on Wayland.

@guidocella
Copy link
Contributor Author

Updated as nanahi requested in #15316. Also, my point in #15316 (comment) of cursor-centric-zoom not needing multiple amounts was moot since you need a negative amount to zoom out (unless you add different script-bindings to zoom in and out).

But unfortunately, beyond the bindings being so long that they get cropped in the stats page, they just don't work, because the script-binding runs before change-list updates the script-opt. So something like repeatable no-osd change-list script-opts append positioning-pan_amount=-0.2; script-binding positioning/pan-x will use the last pan_amount on the first trigger, and only use -0.2 from the second trigger onwards.

@na-na-hi
Copy link
Contributor

input.conf: cycle pause with MBTN_LEFT

This should be put off until dragging deadzone is supported on macOS. It wasn't implemented in that PR.

@Akemi
Copy link
Member

Akemi commented Nov 17, 2024

@na-na-hi did you mean this #13582 one?

@na-na-hi
Copy link
Contributor

@na-na-hi did you mean this #13582 one?

Yes. It makes dragging send a VOCTRL to the VO to signal beginning of dragging, so it has no effect if the VO doesn't handle the VOCTRL.

@Akemi
Copy link
Member

Akemi commented Nov 18, 2024

okay, i will look into it. sry that PR kinda went by me.

@guidocella
Copy link
Contributor Author

How do you intend to support key bindings with different pan and zoom amounts without making them commands? repeatable script-message-to positioning set-pan-amount -0.2; script-binding positioning/pan-x works, but it is a hack which may break if ever support holding down multiple keys to pan diagonally. I think script-binding should just support variable arguments like script-message, else it will always be inferior to commands. It can pass a JSON array of user arguments, mp.input already works this way. (Supporting no-osd would also be nice if feasible).

@na-na-hi
Copy link
Contributor

How do you intend to support key bindings with different pan and zoom amounts without making them commands?

The easiest way would be making fast and slow pan parameters as script options.

I think script-binding should just support variable arguments like script-message, else it will always be inferior to commands. It can pass a JSON array of user arguments, mp.input already works this way.

The problem is that script-binding internally sends the key information as script-message, so any arguments added to script-binding need to be also included in the message which already has some parameters. At best it can only support one argument, because if multiple parameters are supported, we can never add more arguments for the key information again without breaking compatibility.

@guidocella
Copy link
Contributor Author

The easiest way would be making fast and slow pan parameters as script options.

As I just wrote changing script-opts within the binding doesn't work, so we would need 8 script bindings + 2 script-opts just to pan: pan-up, pan-right, pan-down, pan-left, pan-up-slow, pan-right-slow, pan-down-slow, pan-left-slow. And what about when someone wants a 3rd speed? How is this better than 1 command?

The problem is that script-binding internally sends the key information as script-message, so any arguments added to script-binding need to be also included in the message which already has some parameters. At best it can only support one argument, because if multiple parameters are supported, we can never add more arguments for the key information again without breaking compatibility.

Hence the JSON array, you send all user arguments as 1 argument to script message. I already do this in console.lua to send 0 1 or 2 arguments to input.lua, and I can add more arguments whenever is necessary.

@na-na-hi
Copy link
Contributor

I have added support for custom argument for script-binding in #15316. This should make it possible to customize amount in input.conf.

@guidocella guidocella force-pushed the positioning branch 2 times, most recently from 13cdd2c to 8dcc108 Compare November 18, 2024 18:19
@guidocella
Copy link
Contributor Author

This works great now by depending on #15316. You can easily fork the script and there is no need to add commands to the core, and other scripts can also benefit from the new scale and arg arguments to script-binding.

Once macOS supports the dragging deadzone we can bind drag-to-pan to MBTN_RIGHT by default, but there is no hurry.

etc/input.conf Outdated
Comment on lines 61 to 64
#Alt+LEFT script-binding positioning/pan-x -0.2 # pan to the right
#Alt+RIGHT script-binding positioning/pan-x 0.2 # pan to the left
#Alt+UP script-binding positioning/pan-y -0.2 # pan down
#Alt+DOWN script-binding positioning/pan-y 0.2 # pan up
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need to be changed? Could we use Ctrl+Alt modifier for new behavior? Also could update numpad bindings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, up to you if you want to keep video-pan, I never used it and never understood why you would want to move a video beyond the visible window. But it's probably not worth binding both especially since ctrl+alt+arrows is inconvenient to press. I would like to have different default key bindings for images where they can just be bound to arrows but I'm not sure how that could be done if we don't want to use input sections.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think there is no reason to change this when there are spare keys available. Also if scripts are disabled these commands now do nothing which isn't good.

Copy link
Contributor

Choose a reason for hiding this comment

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

ctrl+alt+arrows is inconvenient to press

I don't see any difference to alt+arrows. Can't do that realistically with one hand either.

I would like to have different default key bindings for images where they can just be bound to arrows but I'm not sure how that could be done if we don't want to use input sections.

I was thinking some time ago about this, because lots of key bindings could be different based on image/video context. This is different topic tho.

Also if scripts are disabled these commands now do nothing which isn't good.

This is exactly what I worry too. mpv should be usable without lua backed compiled in, we shouldn't replace core features with script counterpart for important key bindings. Not sure if video-pan- is important, but I would leave it as-is to avoid this issue. For first-party input config we should prefer native / always available features.

Comment on lines +69 to +71
#Ctrl+WHEEL_UP script-binding positioning/cursor-centric-zoom 0.1 # zoom in towards the cursor
#Ctrl+WHEEL_DOWN script-binding positioning/cursor-centric-zoom -0.1 # zoom out towards the cursor
Copy link
Contributor

@kasper93 kasper93 Nov 18, 2024

Choose a reason for hiding this comment

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

Does it need to be changed? Could we use Ctrl+Alt modifier for new behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason not to have cursor-centric-zoom on the wheel? It is more convenient, also according to nanahi ctrl+wheel "makes "pinch to zoom" with touchpads and touchscreens work out of box on Windows, since by default applications receive these key inputs for pinch gesture.", so it needs to be bind to ctrl+wheel to work with pinch gestures by default.

I have no idea what numpad bindings have to do with cursor-centric-zoom, the numpad is not a cursor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to have cursor-centric-zoom on the wheel? It is more convenient, also according to nanahi ctrl+wheel "makes "pinch to zoom" with touchpads and touchscreens work out of box on Windows, since by default applications receive these key inputs for pinch gesture.", so it needs to be bind to ctrl+wheel to work with pinch gestures by default.

Ok, if you think it is more convenient this way. I think it is still valuable to control zoom without messing up video positioning completely on each step. But I guess by default it can be cursor centric.

Note that same issues applies, if lua is disabled in mpv build, it probably should fallback to normal video-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.

You can still use the Alt key bindings for that, we didn't even have Ctrl+wheel until recently.

I removed the Alt+Arrow bindings so you do want to implement a fallback system just for this? I guess we can define alternative bindings with some keyword that input.c can recognize?

#Alt+BS set video-zoom 0; set panscan 0; set video-pan-x 0; set video-pan-y 0 # reset zoom and pan settings
#Ctrl+WHEEL_UP script-binding positioning/cursor-centric-zoom 0.1 # zoom in towards the cursor
#Ctrl+WHEEL_DOWN script-binding positioning/cursor-centric-zoom -0.1 # zoom out towards the cursor
#Alt+BS set video-zoom 0; no-osd set panscan 0; no-osd set video-pan-x 0; no-osd set video-pan-y 0; no-osd set video-align-x 0; no-osd set video-align-y 0 # reset zoom and pan settings
Copy link
Contributor

Choose a reason for hiding this comment

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

Could update numpad bindings the same way.

@@ -588,6 +588,7 @@ static const m_option_t mp_opts[] = {
OPT_CHOICE(lua_load_auto_profiles, {"no", 0}, {"yes", 1}, {"auto", -1}),
.flags = UPDATE_BUILTIN_SCRIPTS},
{"load-select", OPT_BOOL(lua_load_select), .flags = UPDATE_BUILTIN_SCRIPTS},
{"load-positioning", OPT_BOOL(lua_load_positioning), .flags = UPDATE_BUILTIN_SCRIPTS},
Copy link
Contributor

Choose a reason for hiding this comment

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

It is used int defualt input.conf, I don't know if it is good to allow unloading this script. But maybe needed in some cases.

EDIT: We should probably merge all those load- into one string list option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--load options let you use a fork of that script in your scripts directory which was one of the arguments for making this a script. They are also convenient for development without recompiling.

etc/input.conf Outdated
@@ -31,6 +31,7 @@
#MBTN_LEFT ignore # don't do anything
#MBTN_LEFT_DBL cycle fullscreen # toggle fullscreen
#MBTN_RIGHT cycle pause # toggle pause/playback mode
#MBTN_MID script-binding positioning/align-to-cursor # pan the whole image
Copy link
Contributor

Choose a reason for hiding this comment

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

align-to-cursor should not be bound to mouse by default because I can't reset panning with only mouse. At least add a modifier for this.

Copy link
Contributor Author

@guidocella guidocella Nov 18, 2024

Choose a reason for hiding this comment

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

The assumption is that #15320 will merged first, so it won't do anything by default until you zoom in.

Copy link
Contributor

Choose a reason for hiding this comment

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

This also affects panscan and I think the dragging behavior is annoying for the purpose of panscan.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could use MBTN_MID_DBL to reset. I agree though there should be a way to undo the change with mouse only.

Browsers also enter panning mode with middle mouse button, without modifiers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bound MBTN_MID_DBL to reset align and pan. When MBNT_RIGHT will drag-to-pan I guess can we also bind MBTN_RIGHT_DBL to reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MBTN_MID can also be bound only for images, if preferred by users to not affect panscan, though I think it can be useful to pan after panscan and zooming videos.

@guidocella guidocella force-pushed the positioning branch 4 times, most recently from e759b4a to 67ec8ae Compare November 19, 2024 10:57
@guidocella guidocella force-pushed the positioning branch 3 times, most recently from 1581d81 to 3b2dd83 Compare November 24, 2024 08:43
@guidocella
Copy link
Contributor Author

Added an option to toggle align-to-cursor without having to hold down the mouse button, as requested by a user. It retrieves osd-dimensions within the callback now, because the window can change size while align-to-cursor is enabled.

This script provides script bindings to pan videos and images, making
mpv a better image viewer out of the box.
Alt+BS prints "video-pan-y: 0" which seems arbitrary. Make it print
video-zoom: 0 which is the most important property that gets reset.
This is useful because positioning.lua uses video-align.
Probably not worth adding to etc/restore-old-bindings.conf since it's an
extension of add video-zoom.
Also bind MBTN_MID_DBL to allow resetting align and pan with the mouse.
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.

5 participants