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

feat!: reworked config options for fullscreen scale adjustments #664

Merged
merged 13 commits into from
Oct 6, 2023

Conversation

tomasklaen
Copy link
Owner

@tomasklaen tomasklaen commented Oct 3, 2023

Removed options:

timeline_size_fullscreen
controls_size_fullscreen
volume_size_fullscreen
menu_item_height_fullscreen
menu_min_width_fullscreen
top_bar_size_fullscreen

Additionally, ui_scale has been renamed to scale.

The scaling can now be controlled by these two new options:

scale=1
scale_fullscreen=1.3

closes #543

Removed options:

```
timeline_size_fullscreen
controls_size_fullscreen
volume_size_fullscreen
menu_item_height_fullscreen
menu_min_width_fullscreen
top_bar_size_fullscreen
```

Additionally, `ui_scale` has been renamed to `scale`.

The scaling can now be controlled by these two new options:

```
scale=1
scale_fullscreen=1.5
```

closes #543
@dyphire
Copy link
Contributor

dyphire commented Oct 3, 2023

The default value of 1.5 seems a bit large, especially for the top bar. Maybe 1.3 or 1.4 is a more reasonable value.

@tomasklaen
Copy link
Owner Author

True.

@christoph-heinrich
Copy link
Contributor

christoph-heinrich commented Oct 3, 2023

Since we're already touching that, I do still have that poc patch lying round for making the UI grow between window size
limits.

Imagine you have a mpv window open that takes up pretty much the entire screen, but it's not fullscreen or maximized so all elements are still small. Then you fullscreen that window, which makes it only a little bit larger then it used to be, but the elements grow by a factor of 1.3. That doesn't make much sense.
Instead we could linearly interpolate the growing factor based on the window size limits set by the user instead of distinguishing between windowed or not.

In my testing a lower limit of 960x540 and an upper limit of 1920x1080 works well on a 1080p monitor.
Specifying both is probably redundant, setting an upper limit and then having the lower limit be half of that should be fine.

I'm not sure how to deal with different aspect ratios, maybe sqrt(width*height) and then scaling based on that could work well?

@tomasklaen
Copy link
Owner Author

Imagine you have a mpv window open that takes up pretty much the entire screen, but it's not fullscreen or maximized...

But that's almost none's use case. I wouldn't be against triggering the fullormaxed flag if window covers lets say more than 90% of the screen area, but afaik we don't have access to window screen dimensions.

Either way, as I've said before, I don't want UI to scale like that. Sometimes I want to make a window larger to see more of the UI, like more items in the menu or widen the menu to read longer item titles that might be clipped for example, and this kid of scaling would be working against me. Or in tall portrait videos that cut out controls, making the window larger also won't help much if the UI scales like that.

@christoph-heinrich
Copy link
Contributor

Shuts down idea without ever trying it

If doubling the window size increases the element size by a factor of 1.3, then making the window bigger still provides much more room for elements. And with there being limits it's not like it scales indefinitely.

@tomasklaen
Copy link
Owner Author

I just don't see this as a problem that needs solving. Quite the opposite, I generally hate UIs that scale with window, and want consistently sized UI in windowed more. The only place where that kind of makes sense to me are some games, but that's about it. And the solutions to this bother me more than the problem (which, again, is a feature to me).

Pros:

  • Eliminates jump of UI scale for those 2 people that usually toggle between almost fullscreen and fullscreen, and they notice it, and it bothers them...

Cons:

  • Adds more config options.
  • Adds more code.
  • Makes problems outlined in previous post. Even though they're not as jarring as they'd be if we scaled like original osc does.

@christoph-heinrich
Copy link
Contributor

christoph-heinrich commented Oct 3, 2023

It's one option and a few lines in update_display_dimensions() (and a line or two for parsing the option), not that big a deal, but ok I'll drop it.


The way you implemented that makes it suffer from the same problem as hidpi scaling. Instead of scaling the element size it now scales the resolution we render at, as a result element edges that should be at a pixel border are now in the middle of that pixel, resulting in a blurry edge.

The solution to that would be to store the calculated scale somewhere instead of scaling the resolution with it (display.scale or state.scale) and then scale the element size based on that (size = round(options.element_size * display.scale)).
That would also solve the problem for hidpi scaling, there would be no need for display.scale_x/y anymore, and calculations for the thumbnail would become a lot simpler.

I've never changed the way ui scaling was done because I though it would require a ton of changes for each element, but it seems everything is already written to scale based on the element size, and so it would only require changing that??? I might be missing something.

@tomasklaen
Copy link
Owner Author

Yeah I've noticed the slight blur with scale ~= 1 as well, but I guess I decided to suppress it in my head because I liked the simplicity of the current solution :)

I might be missing something.

No, it should work fine. I'll change it to that.

@tomasklaen
Copy link
Owner Author

tomasklaen commented Oct 4, 2023

That should be all. I've also fixed a lot of places where we were not rounding stuff causing blur.

This also enables a trivial border_radius option. I'll add it when this is merged.

@tomasklaen
Copy link
Owner Author

Whops, thumbnails are borked when scale ~= 1. I'll fix it a bit later.

@christoph-heinrich
Copy link
Contributor

The tooltip_margin and tooltip_gap currently don't scale with anything in the timeline, and ass:tooltip() also has some unscaled values, idk if we want to scale them too.

@tomasklaen
Copy link
Owner Author

This should be all.

Copy link
Contributor

@christoph-heinrich christoph-heinrich left a comment

Choose a reason for hiding this comment

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

The volume slider still needs some adjustment. Other then that everything looks good.

diff --git a/scripts/uosc/elements/Volume.lua b/scripts/uosc/elements/Volume.lua
index 950245b..979f0f1 100644
--- a/scripts/uosc/elements/Volume.lua
+++ b/scripts/uosc/elements/Volume.lua
@@ -35,6 +35,7 @@ function VolumeSlider:init(props)
 	self.nudge_size = 0
 	self.draw_nudge = false
 	self.spacing = 0
+	self.border_size = 0
 end
 
 function VolumeSlider:get_visibility() return Elements.volume:get_visibility(self) end
@@ -46,10 +47,16 @@ function VolumeSlider:set_volume(volume)
 end
 
 function VolumeSlider:set_from_cursor()
-	local volume_fraction = (self.by - cursor.y - options.volume_border) / (self.by - self.ay - options.volume_border)
+	local volume_fraction = (self.by - cursor.y - self.border_size) / (self.by - self.ay - self.border_size)
 	self:set_volume(volume_fraction * state.volume_max)
 end
 
+function VolumeSlider:on_display()
+	self.border_size = options.volume_border * state.scale
+end
+function VolumeSlider:on_options()
+	self.border_size = options.volume_border * state.scale
+end
 function VolumeSlider:on_coordinates()
 	if type(state.volume_max) ~= 'number' or state.volume_max <= 0 then return end
 	local width = self.bx - self.ax
@@ -86,8 +93,8 @@ function VolumeSlider:render()
 
 	local ass = assdraw.ass_new()
 	local nudge_y, nudge_size = self.draw_nudge and self.nudge_y or -INFINITY, self.nudge_size
-	local volume_y = self.ay + options.volume_border +
-		((height - (options.volume_border * 2)) * (1 - math.min(state.volume / state.volume_max, 1)))
+	local volume_y = self.ay + self.border_size +
+		((height - (self.border_size * 2)) * (1 - math.min(state.volume / state.volume_max, 1)))
 
 	-- Draws a rectangle with nudge at requested position
 	---@param p number Padding from slider edges.
@@ -154,7 +161,7 @@ function VolumeSlider:render()
 
 	-- BG & FG paths
 	local bg_path = create_nudged_path(0)
-	local fg_path = create_nudged_path(options.volume_border, volume_y)
+	local fg_path = create_nudged_path(self.border_size, volume_y)
 
 	-- Background
 	ass:new_event()
@@ -193,7 +200,7 @@ function VolumeSlider:render()
 
 	-- Disabled stripes for no audio
 	if not state.has_audio then
-		local fg_100_path = create_nudged_path(options.volume_border)
+		local fg_100_path = create_nudged_path(self.border_size)
 		local texture_opts = {
 			size = 200, color = 'ffffff', opacity = visibility * 0.1, anchor_x = ax,
 			clip = '\\clip(' .. fg_100_path.scale .. ',' .. fg_100_path.text .. ')',

@@ -99,7 +99,7 @@ function VolumeSlider:render()

-- Draws a rectangle with nudge at requested position
---@param p number Padding from slider edges.
---@param r number Padding from slider edges.
---@param r number Border radius.
Copy link
Contributor

Choose a reason for hiding this comment

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

Corner radius?

Copy link
Owner Author

Choose a reason for hiding this comment

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

border-radius is kind of a convention.

Copy link
Contributor

@christoph-heinrich christoph-heinrich Oct 6, 2023

Choose a reason for hiding this comment

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

Fair enough, but wouldn't that make the edges of the notch also part of the border? 🤔
I'm not saying we should actually round them, just wondering about the semantics.

@tomasklaen tomasklaen merged commit d2febcb into main Oct 6, 2023
@tomasklaen tomasklaen deleted the fullscreen_rework branch October 9, 2023 12:02
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.

Consolidate all *_fullscreen options into fullscreen_scale_factor
3 participants