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.

Sign up for GitHub

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

Jump to bottom

Geometry classes should support floating-point types #1829

Closed
wmww opened this issue Dec 1, 2020 · 4 comments
Closed

Geometry classes should support floating-point types #1829

wmww opened this issue Dec 1, 2020 · 4 comments
Labels
enhancement

Comments

@wmww
Copy link
Contributor

wmww commented Dec 1, 2020

Rather than re-writing floating-point variants of everything, I propose we change the current types to templates and make specializations of them. PointI would be an alias for Point<int> and an equivalent of the current Point, DisplacementF would be an alias of Displacement<float>, etc. This would give our input handling code the same level of type safety as the rest of Mir.
@wmww wmww mentioned this issue Dec 1, 2020
Merged
bors bot added a commit that referenced this issue Dec 4, 2020
@bors @wmww @AlanGriffiths
Merge #1830
5b9c5b3 
1830: Send sub-pixel input events to Wayland clients r=AlanGriffiths a=wmww

Fixes #1814, fixes #1828. This area of code will be a little nicer when #1829 is done, but I don't think we should block on that. As you can see we use the ugly and rather unsafe `std::pair<float, float>` types to represent sub-pixel positions, but the amount of code that has to handle this is not large.

Another option would be to send the event all the way through until we hit the Wayland wrapper call. This would be a good idea, but it seems pointer events aren't mutable and generic events aren't queryable, so we'd have to do a bunch of messy casting and IMO it's not worth it.

In the long run all this code will disappear once we address #1732 and make subsurfaces real input surfaces.

Co-authored-by: William Wold <wm@wmww.sh>
Co-authored-by: Alan Griffiths <alan@octopull.co.uk>
bors bot added a commit that referenced this issue Dec 4, 2020
@bors @wmww
@AlanGriffiths @Saviq
Merge #1830
05b36ca 
1830: Send sub-pixel input events to Wayland clients r=AlanGriffiths a=wmww

Fixes #1814, fixes #1828. This area of code will be a little nicer when #1829 is done, but I don't think we should block on that. As you can see we use the ugly and rather unsafe `std::pair<float, float>` types to represent sub-pixel positions, but the amount of code that has to handle this is not large.

Another option would be to send the event all the way through until we hit the Wayland wrapper call. This would be a good idea, but it seems pointer events aren't mutable and generic events aren't queryable, so we'd have to do a bunch of messy casting and IMO it's not worth it.

In the long run all this code will disappear once we address #1732 and make subsurfaces real input surfaces.

Co-authored-by: William Wold <wm@wmww.sh>
Co-authored-by: Alan Griffiths <alan@octopull.co.uk>
Co-authored-by: Michał Sawicz <michal.sawicz@canonical.com>
@AlanGriffiths AlanGriffiths mentioned this issue Dec 9, 2020
Merged
@wmww
Copy link
Contributor Author

wmww commented Dec 10, 2020 •
edited

I have an idea for the right way to do this. I started working on it but it's a lot of work so I don't want to continue until I've gotten a green-light for the idea.

  1. Replace IntWrapper<Tag> with a more generic Tagged<Tag, T> with T being the value type.
  2. Use using ... = ... to define generic versions of dimensions: WidthT, HeightT, XT, YT, DeltaXT and DeltaYT (T stands for template, open to other suggestions. We can't use the names directly because they will still mean the int versions for backwards compat)
  3. Define Width, Height, X, etc to be the int versions
  4. Define WidthF, HeightF, XF, etc to be the float versions
  5. Change all the operators to templates
  6. Add conversions that change type but don't change tag
  7. Change Point to more generic PointT<XType, YType>,
  8. Define Point to be the <Width, Height> int version, and PointF to be the float version
  9. Add safe casting to PointT as well
  10. Do 7 - 9 for Displacement, Size and Rectangle

Compared to making entirely new classes for floating points, this would:

  • Lower cost of maintenance, implementation is shared
  • Allow allow for arbitrary types (sometimes we need a SizeT<std::optional<Width>, std::optional<Height>>)
  • Allow safely converting between any types, (It should be easier to go Width -> WidthF than Width -> HeightF). Implementing this for just ints and floats might be possible but it would not scale well to any other types.

Unknowns:

  • Would this be ABI stable? We'd be changing the type signatures but under the hood it should always just compile down to raw numbers.

@AlanGriffiths
Copy link
Contributor 

* Would this be ABI stable? We'd be changing the type signatures but under the hood it should always just compile down to raw numbers.

No. For one thing type names are encoded in function signatures

@wmww
Copy link
Contributor Author

wmww commented Dec 10, 2020

After discussion, we decided we might be able to make generic types work if we change our existing types to inherit from them. I will attempt this and put up a draft PR

@wmww wmww mentioned this issue Dec 10, 2020
Merged
bors bot added a commit that referenced this issue Mar 18, 2021
@bors @wmww
Merge #1858
bb83ae5 
1858: Floating point geometry r=AlanGriffiths a=wmww

Will address #1829 when complete. Adds a new `geometry::generic::Wrapper` class and changes the ABI-stable `IntWrapper` to inherit from it.

Only minor issue is that to be generic, operations need to return `generic::Wrapper<Tag, int>` instead of `IntWrapper<Tag>`. The two can be implicitly converted between, but this can cause a problem in some cases. For example, if `std::max()` is called on an `IntWrapper` and a `generic::Wrapper`, it needs an explicit type to cast to.

When we break ABI, I would like to drop `IntWrapper` and make `Width`, `Height`, `X`, etc typedefs of the `generic::Type<int>`. This would fix those problems.

Co-authored-by: William Wold <wm@wmww.sh>
@AlanGriffiths
Copy link
Contributor

I think this was addressed by #1858

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@AlanGriffiths @wmww