-
Notifications
You must be signed in to change notification settings - Fork 5
[Not merging] Add support for UI scaling #17
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: main
Are you sure you want to change the base?
Conversation
Not exhaustive, but these are the main ones that matter.
…bsoluteUnit.ToPx when necessary
…aling user Canvas commands
|
Ready for review. There's a bit of noise in the diffs since Rider decided to trim all of the trailing whitespace on each line despite my efforts to turn it off. I decided to leave it in since removing it manually was getting annoying and the .editorconfig for Paper enables the option anyway. In hindsight, I think it was the .editorconfig forcing it on. |
| var dotSize = config.Size / 6; | ||
| var spacing = config.Size / 3; | ||
| var dotSize = config.Size.ToPx(scalingSettings) / 6; | ||
| var spacing = config.Size.ToPx(scalingSettings) / 3; |
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 the only thing im not a huge fan of, having the user have to handle the scaling settings in custom drawing manually. Would making a wrapper around a Canvas that just converts the RelativeUnits and AbsoluteUnits to pixels using the scaling setting for the user work?
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.
Thanks for the review!
The Canvas API is fairly large, so I didn't consider wrapping it, but it definitely is doable.
The main cost of this is that we have to remember to update the wrapper if the Canvas API changes.
If a Canvas wrapper is preferred, I can look into implementing this tomorrow.
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.
Maybe instead of a wrapper, we could have a Cast for AbsoluteUnit to a number with the scaling settings when its in the context of a drawing action?
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 see what you mean. Casts don't have parameters and would have to grab the scaling settings from a static variable, which seems too "magical" to me.
Also, going back to the Canvas wrapper idea, I don't think it works either... at least not fully.
AbsoluteUnits default to Points (due to the implicit cast). This is highly beneficial because it ensures UIs scale automatically in most cases.
Rect is in pixels, since I did not change that.
Transform2D is also in pixels for the same reason.
If someone passes a double from a Rect to the Canvas wrapper API, then it's going to be assumed to be in Points, which is incorrect. More information is needed in this case, but more information means manual intervention.
The cleanest option I see is to change Paper to work entirely in terms of Points (I believe web browsers and Avalonia both do this), and make Pixels the exceptional unit (which is technically already is, since it is not the default). This would involve creating the Canvas wrapper or changing Quill itself, ensuring Paper uses it for everything, and update Paper's coordinate system to use Points (Rect, Transform2D, input, resolution).
This would also mean removing AbsoluteUnit again, and just using fractional values to represent physical pixels.
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.
Modifying Quill might make sense, since people might need scaling over there as well anyway.
Just make everything by default be Points, and yeah pixels just a Fraction of a point.
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'm fine with closing this PR and going with the modify Quill approach. I don't think any code in this PR helps with the new approach. I can't say I'll work on it anytime too soon though, since the current solution works for my needs.
I think the main requirements are to ensure that font resolution scales correctly and that enough vertices are output to maintain per-pixel detail. It already mostly works when I modified the projection matrix and resolution that Paper uses (this was the hacky approach I was using before).
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.
Lets leave this PR open for anyone interested, its still valid. Ill look into adding it into Quill when i get a chance.
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.
That sounds good to me. Thanks!
Summary
This PR adds UI scaling support to Paper.
Main public facing changes:
Pointsunit that can be scaled by callingPaper.SetContentScale(double). This is what allows the UI to scale.UnitValuetoRelativeUnit.RelativeUnithandles layout relative units.AbsoluteUnit.AbsoluteUnithandles non-layout relative units (Pixels and Points).UnitValuestatic class that contains constants and helper methods for creating AbsoluteUnits and RelativeUnits.doublesandintsare now converted toPointsunits when typecasting.AddActionElementnow provides theScalingSettingsstruct to facilitate scaling user Canvas commands.Despite these changes, most existing UI code should work with the new API. The main exception is if any user code worked with
UnitValuestruct instances directly since the struct got renamed and theAddActionElementmethod.Main internal changes:
doublesand made sense to scale based on content scale now useAbsoluteUnit.AbsoluteUnitso that those constants scale with content scale. This mainly affects text fields and scrollbars.ScalingSettingsstruct that contains UI scaling information. This struct only contains onedoubleproperty (ContentScale) and only exists for stronger type checking.ScalingSettingsthrough a number of methods so that it can be used to apply UI scaling.AbsoluteUnitandRelativeUnit.Exceptions to content scale scaling. These are documented in the API.
Transformproperty is not scaled.This PR also fixes the issue addressed by #16
For more context
See these messages in the Prowl discord (under the Paper - UI scaling help thread): https://discord.com/channels/1151582593519722668/1413208608572969000/1417892843506110725