Prepare til wrappers for migrating off of SMALL_RECT #11902

lhecker · 2021-12-08T03:11:08Z

This commit makes the following changes to til::point/size/rectangle
for the following reasons:

Rename rectangle into rect
This will make the naming consistent with a later small_rect struct
as well as the existing Win32 POINT/SIZE/RECT structs.
Standardizes til wrappers on int32_t instead of ptrdiff_t
Provides a consistent behavior between x86 and x64, preventing accidental
errors on x86, as it's less rigorously tested than x64. Additionally it
improves interop with MIDL3 which only supports fixed width integer types.
Standardizes til wrappers on throwing gsl::narrow_error
Makes the behavior of our code more consistent.
Makes all eligible functions constexpr
Because why not.
Removes implicit constructors and conversion operators
This is a complex and controversial topic. My reasons are: You can't Ctrl+F
for an implicit conversion. This breaks most non-IDE engines, like the one on
GitHub or those we have internally at MS. This is important for me as these
implicit conversion operators aren't cost free. Narrowing integers itself,
as well as the boundary checks that need to be done have a certain,
fixed overhead each time. Additionally the lack of noexcept prevents
many advanced compiler optimizations. Removing their use entirely
drops conhost's code segment size by around ~6.5%.

References

Preliminary work for #4015.

PR Checklist

I work here
Tests added/passed

Validation Steps Performed

I'm mostly relying on our unit tests here. Both OpenConsole and WT appear to work fine.

carlos-zamora · 2021-12-13T17:55:16Z

src/inc/til/rect.h

@@ -0,0 +1,851 @@
+// Copyright (c) Microsoft Corporation.


(note for reviewers) Use a diff tool (I used text-compare.com) to compare rect.h to rectangle.h and make your life easier haha.

It's probably better in general to check these PRs out locally and compare them VS Code for instance.
BTW VS Code has this which is nice:

carlos-zamora

53/76
Honestly, it all looks good. Most of it seems to be a result of removing the implicit conversion stuff.

src/host/readData.hpp

carlos-zamora · 2021-12-13T19:14:52Z

src/types/UiaTextRangeBase.cpp

@@ -287,16 +287,16 @@ void UiaTextRangeBase::_expandToEnclosingUnit(TextUnit unit)
    // If we're past document end,
    // set us to ONE BEFORE the document end.
    // This allows us to expand properly.
-    if (bufferSize.CompareInBounds(_start, documentEnd, true) >= 0)
+    if (bufferSize.CompareInBounds(_start, documentEnd.to_win32_coord(), true) >= 0)


idk if this is in your other PR, but I'd like this to eventually just be replaced with _start >= documentEnd where both are til::point.

Not really anything to do with your current change, but seems relevant for the "big-picture"

Ah I didn't realize that. I could try and replace it.

Yup! Just about all of the CompareInBounds calls are a little overkill. They actually are the cause of a lot of FAILFAST issues we've had in accessibility code. We may even be able to remove the extra bool parameter off of CompareInBounds if we make this happen. That's why I get really excited as we adopt til::point more in our codebase haha

src/cascadia/TerminalControl/ControlInteractivity.cpp

src/cascadia/TerminalSettingsModel/ActionArgs.idl

carlos-zamora

72/76. Mostly tests are left. I also want to re-review the til::rect changes themselves. Looking good! :)

carlos-zamora · 2021-12-14T19:03:01Z

src/renderer/dx/CustomTextRenderer.cpp

    // Create rectangular block representing where the cursor can fill.
-    D2D1_RECT_F rect = til::rectangle{ til::point{ options.coordCursor } }.scale_up(glyphSize);


scale_up(til::size) seemed useful here. What was the particular reason to remove it?

I found this code in particular so incredibly hard to thoroughly understand that I changed it into this C-style code. In order to understand the existing code you have to know that:

til::point has a constructor for COORD (options.coordCursor)

til::rectangle has a constructor for a point without size, which creates a rectangle at that origin of size 1x1

scale_up multiplies the size of the rectangle, but not the origin

til::rectangle implicitly turns into a D2D1_RECT_F using a conversion operator

The interaction between the second and third point were very confusing for me and it took me more time to understand that code than I'd like to admit. 😄 I'd be fine with this code:

D2D1_RECT_F rect = til::rectangle{ til::point{ options.coordCursor }, glyphSize };

But at that point I asked myself why you'd want to construct a til::rectangle anyways, just to turn it into a D2D1_RECT_F right after. If you'd like me to use that alternative code anyways, I'd be absolutely fine doing so of course! But personally I preferred the C-style code.

carlos-zamora · 2021-12-14T19:23:00Z

src/cascadia/WindowsTerminal/IslandWindow.cpp

-            const auto real = relative / GetCurrentDpiScale();
+            const auto scale = GetCurrentDpiScale();
+            const til::point real{ til::math::flooring, relative.x / scale, relative.y / scale };


Why can't we keep relative/GetCurrentDpiScale()?

The previous code didn't specify the exact til::math::* rounding mode to use. This one does.
This fixes at least one bug in our code where we use til::point / float, which implicitly does a flooring narrow, instead of a more appropriate til::math::round.

carlos-zamora

Finally finished! 🎉🥳 Looks great! Excited to see this land 🙂

Most of the changes are really just...

til::rectangle --> til::rect
implicit --> explicit conversion

So going through most of these files are pretty easy. Anything that isn't that, I've pointed out and you've done a good job of explaining your reasoning.

I like details::extract. It's a nice way to simplify a lot of the assignment and add some "built-in" validation.

Great work my dude 👍

lhecker · 2021-12-18T01:12:48Z

I've rebased the last commit a few times so that there's only one additional commit to review since my initial one.
Apart from "addressing feedback" it also fixes compilation issues, makes the last remaining til constructor explicit (after quite some deliberation tbh) and fixes til::math, which - as it turns out - didn't throw during narrowing. Now it does.

zadjii-msft

You know, I'm not sure I'm qualified to be the person to give the second ✔️ on the til stuff. Most of this looks fine. The GenerateTests.ps1 one definitely needs fixing. But I think I'd be more comfortable with @miniksa / @DHowett's approval, since they wrote most of these originally.

I personally really liked the implicit conversions, I feel like that was a part of what made til so magic. But if we need to get rid of them, then I get it. So long as there's still magic constructors from anything to the til types, then I'll be happy.

zadjii-msft · 2022-01-05T13:30:14Z

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

+        const auto distanceSquared = dx * dx + dy * dy;
+        const auto maxDistanceSquared = w * w / 16; // (w /4)^2
+
+        if (distanceSquared >= maxDistanceSquared)


is there any meaningful difference in behavior (besides just not doing a sqrt call) here by comparing squared distances vs the original?

No. Unless I made a weird mistake, the functionality is exactly the same. 🙂
The change is necessary since the old code wouldn't compile otherwise. Since a change was necessary I changed the distance check to a simpler standard distance check. In my experience it's rather unusual to use sqrt for that.
It's safer to just square the other side of the equation - no NANs & no explicit static_cast casts (which would've been necessary here).

src/interactivity/win32/ut_interactivity_win32/GeneratedUiaTextRangeMovementTests.g.cpp

miniksa · 2022-01-06T22:29:52Z

I personally really liked the implicit conversions, I feel like that was a part of what made til so magic.

I agree with this sentiment.

But if we need to get rid of them, then I get it. So long as there's still magic constructors from anything to the til types, then I'll be happy.

But after Leonard was explaining to me that the implicits were making a whole bunch of errors and thrashing... I can live with the change to be more explicit. I meant to eliminate bug factories with the til types, not turn them into one.

src/inc/til/point.h

src/inc/til/size.h

src/inc/til/point.h

src/cascadia/TerminalControl/TermControl.cpp

src/cascadia/TerminalSettingsModel/ActionArgs.idl

src/renderer/dx/DxRenderer.cpp

src/terminal/adapter/FontBuffer.cpp

src/til/ut_til/SPSCTests.cpp

src/inc/til/rect.h

…5-prepare

lhecker · 2022-01-12T00:40:59Z

Sorry for the force-push! I wanted to ensure that people who have reviewed before only need to review a single new commit.
It has the following changes:

Address feedback you all brought up
Pass point/size by value (fits into a single register) and rect by reference (doesn't fit) (until MSVC gets smart about this)
Fix compilation of WddmCon/Bgfx engines (hopefully - I didn't test it)
Simplify some of the code changes

miniksa

Let's do this!

…5-prepare

ghost · 2022-01-13T21:09:07Z

Hello @lhecker!

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 · 2022-02-03T18:22:57Z

🎉Windows Terminal Preview v1.13.10336.0 has been released which incorporates this pull request.:tada:

Handy links:

lhecker added this to the Engineering Improvements 2021 milestone Dec 8, 2021

lhecker force-pushed the dev/lhecker/issue-4015-prepare branch 2 times, most recently from 99e7ea2 to 568c52b Compare December 9, 2021 17:10

Prepare til wrappers for migrating off of SMALL_RECT

21e912d

lhecker force-pushed the dev/lhecker/issue-4015-prepare branch from 568c52b to 21e912d Compare December 9, 2021 17:18

lhecker marked this pull request as ready for review December 9, 2021 17:18

carlos-zamora reviewed Dec 13, 2021

View reviewed changes

carlos-zamora reviewed Dec 14, 2021

View reviewed changes

carlos-zamora approved these changes Dec 15, 2021

View reviewed changes

lhecker force-pushed the dev/lhecker/issue-4015-prepare branch from cbbed89 to 6b0f328 Compare December 18, 2021 00:24

This comment has been minimized.

Sign in to view

Address feedback

1bf8011

lhecker force-pushed the dev/lhecker/issue-4015-prepare branch from 6b0f328 to 1bf8011 Compare December 18, 2021 00:29

zadjii-msft self-assigned this Jan 4, 2022

zadjii-msft reviewed Jan 5, 2022

View reviewed changes

zadjii-msft assigned DHowett and miniksa and unassigned zadjii-msft Jan 5, 2022

miniksa reviewed Jan 6, 2022

View reviewed changes

lhecker added 2 commits January 11, 2022 21:44

Merge remote-tracking branch 'origin/main' into dev/lhecker/issue-401…

4a5587a

…5-prepare

Address feedback, Fix annoyances

9037412

lhecker force-pushed the dev/lhecker/issue-4015-prepare branch from e949c5b to 9037412 Compare January 12, 2022 00:37

lhecker requested a review from miniksa January 12, 2022 00:41

miniksa approved these changes Jan 12, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/lhecker/issue-401…

47fc9a4

…5-prepare

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 13, 2022

ghost merged commit b3fab51 into main Jan 13, 2022

ghost deleted the dev/lhecker/issue-4015-prepare branch January 13, 2022 21:09

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare til wrappers for migrating off of SMALL_RECT #11902

Prepare til wrappers for migrating off of SMALL_RECT #11902

lhecker commented Dec 8, 2021 •

edited

Loading

carlos-zamora Dec 13, 2021

lhecker Dec 13, 2021

carlos-zamora left a comment

carlos-zamora Dec 13, 2021

lhecker Dec 13, 2021

carlos-zamora Dec 14, 2021

carlos-zamora left a comment

carlos-zamora Dec 14, 2021

lhecker Dec 15, 2021 •

edited

Loading

carlos-zamora Dec 14, 2021

lhecker Dec 15, 2021

carlos-zamora left a comment

This comment has been minimized.

lhecker commented Dec 18, 2021

zadjii-msft left a comment

zadjii-msft Jan 5, 2022

lhecker Jan 10, 2022 •

edited

Loading

miniksa commented Jan 6, 2022 •

edited

Loading

lhecker commented Jan 12, 2022

miniksa left a comment

ghost commented Jan 13, 2022

ghost commented Feb 3, 2022

		// Create rectangular block representing where the cursor can fill.
		D2D1_RECT_F rect = til::rectangle{ til::point{ options.coordCursor } }.scale_up(glyphSize);

Prepare til wrappers for migrating off of SMALL_RECT #11902

Prepare til wrappers for migrating off of SMALL_RECT #11902

Conversation

lhecker commented Dec 8, 2021 • edited Loading

References

PR Checklist

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

This comment has been minimized.

lhecker commented Dec 18, 2021

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

miniksa commented Jan 6, 2022 • edited Loading

lhecker commented Jan 12, 2022

miniksa left a comment

Choose a reason for hiding this comment

ghost commented Jan 13, 2022

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 commented Feb 3, 2022

lhecker commented Dec 8, 2021 •

edited

Loading

lhecker Dec 15, 2021 •

edited

Loading

lhecker Jan 10, 2022 •

edited

Loading

miniksa commented Jan 6, 2022 •

edited

Loading

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.