-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
console.lua: scale with the window height #14114
base: master
Are you sure you want to change the base?
Conversation
Download the artifacts for this pull request: Windows* [mpv-i686-w64-mingw32](link) * [mpv-x86_64-w64-mingw32](link) * [mpv-x86_64-windows-msvc](link)macOS* [mpv-macos-12-intel](link) * [mpv-macos-13-intel](link) * [mpv-macos-14-arm](link) |
player/lua/console.lua
Outdated
w = 720 * aspect | ||
end | ||
|
||
local scale = mp.get_property_native("display-hidpi-scale", 1) * opts.scale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSD and other scripts don't scale contents with hidpi scale in either scaling modes, so it still behaves differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we remove it then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also removed scale
since I don't understand its purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scale
is basically required for readability on a hidpi display for lower-resolution videos. Well for me at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why we have to be the only application to scale text with window height either and use --osd-scale-by-window=no
. but both kasper and nanahi requested this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You guys are moving the bar right now. Everything in mpv scales. OSD, OSC, stats, subtitles. Name one thing which does not scale... console. I requested this, because it makes sense in mpv context.
EDIT:
If it is undesirable behavior, we can add another option and make it disable by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh there could be an argument made for subtitles not scaling. But anyways, I don't see the advantage of this change. It's fine to have it as an option I guess, but to me it's just a strictly worse default imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this scaling only makes sense to not break ASS subtitles and they are not affected by --sub-scale-by-window
anyway (despite the docs stating "Like --sub-scale, this can break ASS subtitles."). I don't understand the advantage of scaling by default elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stay on topic please. If you don't like the defaults, lets take effort to make OSC and stats respect --osd-scale-by-window
by default, then changing the default for everything consistently only needs to change the default for a single flag. This PR is a step towards that.
c7ed931
to
06ae3fb
Compare
8187ebf
to
e948a0d
Compare
If --osd-scale-by-window is true scale console.lua with the window height to be consistent with the OSD, osc.lua and stats.lua. Also remove the scale option because you can set font_size and border_size to have the same effect. display-hidpi-scale is still used and it may make sense to also multiple other mpv text sizes by it.
e948a0d
to
53bcc46
Compare
This makes console.lua consistent with the OSD, osc.lua and stats.lua. This reads --osd-scale-by-window so users don't have to configure vidscale in yet another script.