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

til::size #4850

Merged
11 commits merged into from
Mar 10, 2020
Merged

til::size #4850

11 commits merged into from
Mar 10, 2020

Conversation

miniksa
Copy link
Member

@miniksa miniksa commented Mar 9, 2020

Summary of the Pull Request

Introduces convenience type til::size which automatically implements our best practices for size-related types and provides automatic conversions in/out of the relevant types.

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • Automatically converts in from anything with a X/Y (console COORD) or cx/cy (Win32 SIZE)
  • Automatically converts out to COORD, SIZE, or D2D1_SIZE_F.
  • Constructs from bare integers written into source file
  • Default constructs to empty
  • Uses Chromium Math for all basic math operations (+, -, *, /)
  • Provides equality tests
  • Adds initial proposal for division-to-ceiling (round up division) that attempts to ceil without any floating point math.
  • Accessors for height/width
  • Type converting accessors (that use safe conversions and throw) for height/width
  • Convenience function for area calculation (as that's common with type) and uses safe math to do it.
  • TAEF/WEX Output and Comparators so they will print very nicely with VERIFY and Log macros in our testing suite.

Validation Steps Performed

  • See automated tests of functionality.

src/inc/til/size.h Outdated Show resolved Hide resolved
@miniksa
Copy link
Member Author

miniksa commented Mar 9, 2020

Nnnnn I don't think I checked SA here and it's not used in an SA-enabled project yet. I should do that.

@miniksa
Copy link
Member Author

miniksa commented Mar 10, 2020

OH. I should put a natvis on this too, eh, @DHowett-MSFT?

@DHowett-MSFT
Copy link
Contributor

i do like me some good natvisses

src/inc/til/size.h Show resolved Hide resolved
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

  1. Is this possible to use in a clamped context?
  2. Should this maybe be til::vec2 that exposes both width() and x() (for example)?

@zadjii-msft zadjii-msft added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Meta The product is the management of the products. labels Mar 10, 2020
@carlos-zamora
Copy link
Member

carlos-zamora commented Mar 10, 2020

Is there a way I use this for clamped math? Would I just have to catch the HRESULT after each operation? It'd be nice if I could just say "always clamp to these dimensions" (i.e. text buffer size), and it would "just work". But that might also be outside the scope of this PR. Thoughts?

EDIT: derp. zadji mentioned it in the comment just before mine...

@miniksa
Copy link
Member Author

miniksa commented Mar 10, 2020

1. Is this possible to use in a clamped context?

No, not yet. I didn't need that yet... so I didn't put it in.
This isn't really intended to be the END of writing til::size. The beginning and what I needed for rendering. I just didn't want to go off the deep end and be writing the support class forever so I did just about as much as I needed then sent it in.

2. Should this maybe be `til::vec2` that exposes both `width()` and `x()` (for example)?

What you're not seeing yet is that til::point is coming for x/y. And they don't inherit or whatever from each other because I wanted the distinction for til::rectangle to construct with either point/point (as the two corners) or point/size (as the origin and the dimensions).

@miniksa
Copy link
Member Author

miniksa commented Mar 10, 2020

Is there a way I use this for clamped math? Would I just have to catch the HRESULT after each operation? It'd be nice if I could just say "always clamp to these dimensions" (i.e. text buffer size), and it would "just work". But that might also be outside the scope of this PR. Thoughts?

I don't have a clamped one yet because I didn't have a specific use case in my mind yet and I was trying not to write this class forever. If we have a use for clamped, then we can add it. This isn't the end of writing til::size. We'll add more stuff to it as we need it.

Maybe it takes an optional bool parameter on the constructors that sets it to clamped mode instead of checked mode? We can figure that out in a future revision, IMO.

@miniksa miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 10, 2020
@ghost
Copy link

ghost commented Mar 10, 2020

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 57ee5a9 into master Mar 10, 2020
@ghost ghost deleted the dev/miniksa/til_size branch March 10, 2020 20:51
abhijeetviswam pushed a commit to abhijeetviswam/terminal that referenced this pull request Mar 12, 2020
## Summary of the Pull Request
Introduces convenience type `til::size` which automatically implements our best practices for size-related types and provides automatic conversions in/out of the relevant types.

## PR Checklist
* [x] In support of Differental Rendering microsoft#778
* [X] I work here.
* [x] Tests added/passed
* [x] I'm a core contributor.

## Detailed Description of the Pull Request / Additional comments
- Automatically converts in from anything with a X/Y (console `COORD`) or cx/cy (Win32 `SIZE`)
- Automatically converts out to `COORD`, `SIZE`, or `D2D1_SIZE_F`.
- Constructs from bare integers written into source file
- Default constructs to empty
- Uses Chromium Math for all basic math operations (+, -, *, /)
- Provides equality tests
- Adds initial proposal for division-to-ceiling (round up division) that attempts to `ceil` without any floating point math.
- Accessors for height/width
- Type converting accessors (that use safe conversions and throw) for height/width
- Convenience function for area calculation (as that's common with type) and uses safe math to do it.
- TAEF/WEX Output and Comparators so they will print very nicely with `VERIFY` and `Log` macros in our testing suite.

## Validation Steps Performed
- See automated tests of functionality.
This was referenced Mar 12, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met Product-Meta The product is the management of the products.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants