-
Notifications
You must be signed in to change notification settings - Fork 443
Expose tablet OutputSize #6460
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
base: master
Are you sure you want to change the base?
Expose tablet OutputSize #6460
Conversation
@@ -38,6 +38,8 @@ public class OpenTabletDriverHandler : InputHandler, IAbsolutePointer, IRelative | |||
|
|||
public Bindable<Vector2> AreaSize { get; } = new Bindable<Vector2>(); | |||
|
|||
public Bindable<Vector2> OutputSize { get; } = new Bindable<Vector2>(); |
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.
Not sold on this API design. I'd say this should be nullable, and if the output size is null, then it is presumed that the output area is the whole window (which would fall back to the old logic). Curious of other thoughts on this though @ppy/team-client
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.
OutputAreaSize
now has a default value of Vector2(1f, 1f)
. So it behaves correctly on first launch.
OutputAreaPosition
does not have a default value, but it's unused until the scaling mode is changed, at which point it will have been populated. I could give it a default of Vector2(0.5f, 0.5f)
if that would be preferable.
I'm also open to making them nullable if that would be better.
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.
Looking at things in their current state, I think I'm fine with the proposed API? Better than nullable values IMHO.
Seems like a duplicate of #6166? |
That's a good catch. I can close this and instead use #6166 as a base for ppy/osu#31141 if that would be preferable. Their PR is more complete as it also includes the output position, which may be useful for future configurability. |
…een-scaling-tablet-output # Conflicts: # osu.Framework/Input/Handlers/Tablet/ITabletHandler.cs # osu.Framework/Input/Handlers/Tablet/OpenTabletDriverHandler.cs
…een-scaling-tablet-output # Conflicts: # osu.Framework/Input/Handlers/Tablet/ITabletHandler.cs # osu.Framework/Input/Handlers/Tablet/OpenTabletDriverHandler.cs
… screen-scaling-tablet-output # Conflicts: # osu.Framework/Input/Handlers/Tablet/ITabletHandler.cs
#6166 is too outdated to work off of directly due to conflicts with osu.Game. So I've:
Apologies, this has made the commit history a little messy. I can rebase if desired, but I'll leave it as-is for now since |
else | ||
tablet.Value = null; |
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.
what is this change? this removal does not look correct in this context
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.
Apologies, I missed this when merging in #6166, which resulted in some conflicts due to being outdated. I've now addressed this.
aside from the above this looks roughly correct, but was there even any testing done here? no changes to test scenes to even add sliders to play with this framework side or anything, so i question whether this was tested to work at all. |
I assumed cross-testing with ppy/osu#31141, which looks to be working well for at least the user. I haven't personally tested, if we can have some minimal framework tests that definitely would be appreciated. |
I've cross-tested with ppy/osu#31141 and included a video in that PR demonstrating the new behaviour. I included a note in that PR asking for advice on how to do testing (since automated testing would require a way to mock tablet input). But I should have included that note on this PR as well. I'll look into adding some sliders to the test framework, so that it can be tested manually. Edit: Adding sliders to osu-framework's visual testing was a great callout. The behaviour isn't quite what I was expecting on the framework side, and I was compensating for it on the osu-game side. I'll temporarily draft this PR, and will aim to work out a better solution shortly after New Years. |
…ling-tablet-output # Conflicts: # osu.Framework.Tests/Visual/Input/TestSceneTabletInput.cs
y => tabletHandler.OutputAreaOffset.Value = new Vector2( | ||
tabletHandler.OutputAreaOffset.Value.X, y)); | ||
|
||
AddSliderStep("change rotation", 0, 360, 0f, |
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.
This is out of scope, but I needed to test that my changes wouldn't break with rotation. I could break this into a separate PR if you'd like, but I figure it's a simple enough addition to include here.
Sorry it's taken so long to get back to this. It's much later than the "Shortly after New Years" that I promised 😅 Changes since the last review include:
On the note of the default values, I found that when restarting the test framework, if there are no sliders for the values, the last used values will be used instead of the default values. I'm assuming there's a config file somewhere storing these values that I should be clearing when testing. I'm currently trying to figure out where that is, so I can validate that the default values will work correctly on a fresh or existing installation. One thing this PR hasn't changed is the tablet area visualization. Output area and rotation are not reflected in the visualization. Edit: I've now included a tablet area visualization for output area. |
As a bit of a tangent, I implemented OutputAreaOffset so that the output area remains within the bounds of the screen. (0, 0) is aligned to the top-left, and (1, 1) is aligned to the bottom-right. This is different from the existing input area implementation, where you position the center of the area directly, allowing the input area to be partially outside the tablet. I think it would make sense to adjust the input area to fix this behaviour. But it would cause differences for anyone who has changed their offset unless we could migrate saved config values. It's outside the scope of this PR, but figured I'd mention it. |
@@ -40,7 +41,8 @@ public TestSceneTabletInput() | |||
Children = new Drawable[] | |||
{ | |||
tabletInfo = new SpriteText(), | |||
areaVisualizer = new TabletAreaVisualiser(), | |||
inputAreaVisualizer = new TabletAreaVisualiser(Vector2.One, "Input area", false), | |||
outputAreaVisualizer = new TabletAreaVisualiser(new Vector2(256, 144), "Output area", true), |
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.
256x144 is just an arbitrary 16:9 scale size for visualization purposes.
}, true); | ||
AreaSize.DefaultChanged += fullSize => fullArea.Size = fullSize.NewValue; | ||
fullArea.Size = AreaSize.Default; | ||
AreaSize.DefaultChanged += _ => updateVisualiser(); |
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.
When the area is confined, position depends on size. Moving all calculations to a single function was simpler.
Resolves #6150
Related to ppy/osu#12098
Required by ppy/osu#31141
Description
Exposes new bindables
OutputAreaSize
andOutputAreaOffset
, so thatabsoluteOutputMode.Output
can be updated from osu.Game.Includes new testing sliders for output area and rotation. Updated input area visualization to show rotation, and added a new visualization for output area.
