command: add cursor-centric-zoom #15310
Conversation
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 after binding cursor-centric-zoom by default.
|
Download the artifacts for this pull request: |
|
This should be done as a built-in/TOOLS script. There is no reason for this to be a command when everything else uses writable properties for zooming. |
|
I think it's a good idea to have a way to provide cursor centric zoom without external scripts. We can either do it with this PR, or a built-in script. But I don't really know why we would add more built-in scripts when a C implementation is already done |
Providing it as a script has several advantages:
The goal of mpv is to be an extensible player, by making more functionalities possible to be implemented as scripts, instead of making everything going into C code, especially for "nice to have", non-essential features like this. |
Not applicable here. I also don’t think memory-safe languages simplify programming; they merely prevent you from accessing memory incorrectly. C is very simple language to work with.
If it is a built-in script, the interface will be bloated anyway.
Depending on the use case, users generally don't need to modify these basic commands.
I kind of agree. All this command does is set already existing properties; it doesn't need to be in C. There are many similar cases where we could move some functionality to scripts for better flexibility. In this case, I would see it as a built-in script that computes correct values for the video-zoom and video-align properties. Such a script could also be extended to include video-dragging functionality, not just zooming. While it's valid to implement this in C, it's a decision whether we want some things directly implemented or if we should rely on the scripting backend and keep the core untouched. But since, it's implemented in C, I don't mind having it, if it's easier this way. |
What reasonable modifications do you envision being made to "cursor centric zoom" without it being a totally different feature entirely? I'm ignoring the first two points on your bullet list because I don't believe even you seriously believe they're good points to bring up, please keep the trolling to a minimum next time. |
|
Extending internally with scripts does keep the core from bloating, and memory-safe languages do have less footguns than C. Just think how much care would be needed to write the OSC or ytdl-hook in C, while in lua, it's maybe not always perfect, but at least you know it's safe (as far as memory access goes), and trivial for users to start hacking. So generally, keep using scripts to extend the code does have advantages. However, in this case I think the functionality is simple enough to keep at the C code. |
What's lua's drawback compared to C here? There is no reason to use C when lua works for this case.
Not really. The command and options of playback core are unchanged. This is what had been done with SMTC, which is a built-in plugin instead of in player core.
This is my plan. I will open a PR to implement them as a built-in script. It will be better than the C implementation for various reasons noted.
For example, smooth/"kinetic" zooming with mouse wheel, or setting to control whether it centers to cursor or screen when zooming to <100% etc.
Please keep bad faith arguments to a minimum next time. This is not constructive to the discussion. |
|
Some points to consider:
|
If it's implemented as a script, this can simply be a script option.
These are playlist management features that are expected to be a built-in player functionality for any video players, and in the loadfile case it reuses existing directory loading code, so they get a pass. A nonessential feature like this PR shouldn't be treated the same.
This would be a better solution than the C one IMO. I don't see any problem with upstreaming built-in lua scripts when they enhance the player with useful features. |
|
Do you know if there's a way to respect cmd->scale in Lua? That would be the only disadvantage of a script. We probably have to add support to script-message itself. |
Not currently, but I have been looking into this. Several script features don't support scaled commands right now but I'm planning to add them. As you said this can be added to script-message. Edit: Scripts can now add scalable key bindings with #15316, so your scripts can handle scaled input devices now. |
The functionality will be unavailable if mpv is compiled without lua. Of course this is rare, but I don't see why we would opt for a Lua implementation when a C implementation is already done. mpv historically has always preferred C implementations except for things that are intended to be modular, like osc or stats.lua or console.lua. This is a command, it is not intended to be modular. If there are improvements to be made to it, it should be done inside mpv. Another reason would be that this increases the maintenance burden in the future when/if luajit stops being actively developed, it's yet another script that will need to be tested to work with Lua 5.3/5.4. This is something we've already discussed before. You don't have to deal with any of these problems if it's implemented in C. Of course, none of this matters if using a built-in Lua script means we can merge more features into mpv beyond just cursor centric zoom. I was in favor of this MR purely because the work is already done, so it makes no sense to add a Lua script just for this. I like #15314 better in any case.
You're engaging in circular reasoning here. You can't say "better for reasons noted" when those reasons are a point of contention. We're discussing the reasons you've cited, you can't say "my reasons are valid for the reasons noted".
This is subjective and has changed over time in this project, I don't believe you get to unilaterally decide where the line is drawn for "non-essential". |
|
For the record, upgrading to Lua 5.4 is easy, I already showed all that's necessary in #14905 and it is just +16 -15 lines. positioning.lua shouldn't need any modification since it doesn't use string.format. |
|
My input will be about the Honestly, a welcomed addition. mpv is capable of being a great image viewer, it is why I suggest guido's centric zoom in my osc's image mode https://github.com/Samillion/ModernZ/blob/main/docs/IMAGE_VIEWER.md To be even more blunt, I genuinely think To be fair, I realize this has never been a high demand, but I honestly think it's because most users aren't aware of such capabilities to be possible within mpv. Also, while my use case suggests "image mode", I'm sure many would appreciate it in all modes as well. It's one of those things that you realize you need, only once you know about it. |
Touch input is untested.