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

Proposal: C# projections of Point, Rect, Size should not pretend to be double-precision #2047

Closed
benstevens48 opened this issue Mar 1, 2020 · 59 comments
Labels
bug Something isn't working team-Markup Issue for the Markup team

Comments

@benstevens48
Copy link

In C#, it looks like Point, Rect and Size have double precision since all their properties are doubles. But in fact this is not the case, they are actually single precision. This is very confusing and extremely bad practice in my opinion. In case anyone wants to verify this, try the following code.

double x = (double)float.Epsilon / 4.0;
System.Diagnostics.Debug.WriteLine(x);
Point p = new Point(x, 0);
System.Diagnostics.Debug.WriteLine(p.X);

It prints

3.50324616081204E-46
0

(Note that you may be thinking the above number is laughably small, but in practice when working at sensible scales of around 100-1000 for layout pixels, the precision of floats drops to something more like 0.0001, I just couldn't be bothered to work out the values for the example in that case).

Most people will not realise the fact that these structs are single-precision which can lead to lots of problems. For example, a while back I tried to store GPS coordinates in a Point struct since I was already using the Windows.Foundation namespace and it seemed convenient. However, it turns out that they lost precision and it look me ages to work out why. Another example is that I wrote some code to do some coordinate geometry calculations, which involved adding a tolerance to allow for floating-point arithmetic errors. I had to make this tolerance a lot larger than I expected, and I never understood why at the time, but it turns out it's because I was using Points which I thought were double-precision but were actually single.

I propose that the C# protections of Point, Rect, Size should have properties that are floats. (As an aside, in my opinion it was a mistake to use single rather than double precision for layout properties like these in the first place because errors are uncomfortably large, but I suppose that decision is made now).

@benstevens48 benstevens48 added the feature proposal New feature proposal label Mar 1, 2020
@msft-github-bot msft-github-bot added this to Freezer in Feature tracking Mar 1, 2020
@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Mar 1, 2020
@jtorjo
Copy link

jtorjo commented Mar 2, 2020

In C#, it looks like Point, Rect and Size have double precision since all their properties are doubles. But in fact this is not the case, they are actually single precision. This is very confusing and extremely bad practice in my opinion. In case anyone wants to verify this, try the following code.

I more than second that. It took me quite a while to figure this one on myown -- since I also assumed they would be double, only to see that they are not. So basically, any set you do is converted to float.

@marcelwgn
Copy link
Contributor

I think the problem is that, when using a Point in XAML, specifying a float (a.k.a Single) is not supported/working.

From the documentation:

If you want a DP with a floating-point type, then make it double (Double in MIDL 3.0). Declaring and implementing a DP of type float (Single in MIDL), and then setting a value for that DP in XAML markup, results in the error Failed to create a 'Windows.Foundation.Single' from the text ''.

So my guess is that, in order to support using points in XAML, they defined them as double in the abstraction layer.

@ranjeshj ranjeshj added the team-Markup Issue for the Markup team label Mar 2, 2020
@danzil danzil added area-DotNet bug Something isn't working and removed needs-triage Issue needs to be triaged by the area owners feature proposal New feature proposal labels Mar 18, 2020
@danzil
Copy link

danzil commented Mar 18, 2020

@stevenbrix will this be resolved by the new projection work?

@benstevens48
Copy link
Author

Thanks for following this up. I just wanted to point out that it would be a breaking change to turn the properties into floats, which I am fine with (and I obviously would't have posted the issue if I wasn't), but it would be good to add it to a documented list of breaking changes somewhere. Unless of course you plan to make the underlying object really store double-precision values.

@stevenbrix
Copy link
Contributor

stevenbrix commented Mar 18, 2020

will this be resolved by the new projection work?

Not sure, I'll keep note of it. Fixing this would either be a source-level or runtime-level breaking change.

This is interesting, the documentation here says it should be a float: https://docs.microsoft.com/en-us/uwp/api/windows.foundation.point.x

@MikeHillberg
Copy link
Contributor

The motivation for Point to be singles was perf. We looked somewhere along the line and single was predominately enough precision, and double caused a measurable perf hit, so we tried to generally make things single internally. At the same time there’s an API guideline (both in .Net and Windows) to prefer double, because languages like double better than single (ex var num = 0.0 in C# makes num a double).

The result of those opposing motivations is that for Silverlight and Xaml (WinUI), some types are single internally but project in the public API as double. For example FrameworkElement.Width is a double in the API but a float internally. On the other hand MapControl.ZoomLevel, where the precision really matters, it’s really a double internally.

When the Windows Point type was originally designed, it was left as a float, and you can see it that way in C++. In C#, though, it still projects as a double for compatibility. There’s a note on Point’s doc page about that (but not on the field's doc page that stevenbrix linked).

(Fyi, there’s also a PointInt32 type that’s used in a couple of places which is Int32 bottom to top.)

@Scottj1s
Copy link
Member

@stevenbrix, you linked to the UWP docs to show 'float X'. But the dotnet docs are double precision. So I don't think there's any issue there, other than it being a bit confusing to show C#, since the projection hides that.

The existing projection for Point, Size, Rect carries over to the Windows.Foundation namespace, rather than using the double precision System.Windows types. Given that, I'm open to fixing this by preserving the W.F mapping but making the types single precision. It's a breaking change, but a rare opportunity.

@danzil, per @chingucoding's observation, would the Xaml compiler/runtime need to change to accommodate single precision structs?

@jtorjo
Copy link

jtorjo commented Mar 20, 2020

It's a breaking change, but a rare opportunity.

That would be awesome, a lot of possible truncation bugs will go away.

@MikeHillberg
Copy link
Contributor

But the docs don't take you there, they take you to the one that stevenbrix linked. (For example, start at PointAnimation and follow the link to Point.)

We're working hard not to treat WinUI 3 as an opportunity to make breaking changes. There are things we have to break for technical or timing reasons, so we've avoided any breaking change pain that's avoidable. (For example this becomes a compiler error: point.X = 4.2;) Couldn't we make this change in a subsequent release?

@Scottj1s
Copy link
Member

Yes, the UWP docs are a bit confusing as a starting point, since they have no awareness of C# projection behavior.

For C#/WinRT, we've been proceeding on the assumption that users will have to rebuild and possibly rewrite a bit (e.g., dropping the winrt interop helpers). It sounds like WinUI is being more conservative, and that's a challenge where the two intersect. I think breaking changes will be harder the longer we go, as more dependent sources accumulate.

@benstevens48
Copy link
Author

As a developer, I can say that migrating to WinUI 3.0 would be the main point at which I would expect to have to make changes to my code, so I would be in favour of making breaking changes at that stage. I would find it more confusing to have to make changes in WinUI 3.1 or something.

However, I understand that you might have decided on the approach of making as few breaking changes as possible in WinUI 3.0, in order to encourage people to migrate to WinUI 3.0 (because you don't want to be left with a lot of apps still using the controls in the Windows namespace else maybe you'd have to put more effort into supporting them).

Overall, I think that all the new stuff coming up, e.g. .net 5, WinUI 3, presents a good opportunity to make some breaking changes like this at a time when developers expect it. If your plan is to make breaking changes in a future release, it would be good to make that very clear in the roadmap, and put it in any visuals you have, so that developers are prepared. For example

  • WinUI 3.0 - minimal breaking changes except namespace from Windows -> Microsoft.
  • WinUI 3.1 - other (limited number of) breaking changes planned

One thing I think we agree on is that the current behavior of Point etc in C# is confusing! (I do sort of understand the original motivation though, but not sure it was the best decision...).

As an aside, which probably won't be helpful in any way at this stage, do the performance gains of using float instead of double for points still exist, given that most people will now be running 64-bit devices? I know that there are some special CPU instructions that allow faster computations with floats, but to what extent is that exploited? (I also understand that eventually things are passed to Direct2D, which uses floats in general (and GPUs are definitely faster with single-precision calculations), but the conversion could maybe have happened at this interface.)

@benstevens48
Copy link
Author

Also, regarding the point about the XAML compiler needing to be changed, I don't know the answer to that, but I did post an issue for it at the same time as I posted this one, in case it was related - see #2048.

@weltkante
Copy link

weltkante commented Mar 20, 2020

These are API mistakes that need to be fixed, the sooner the better. If you don't fix it for sake of compatibility that would be a major sign that the next WinUI is just an interim solution. It basically would be dead before it gets released because it is already clear it needs another generation of XAML framework to get the design right. Thats not exactly going to pull people towards using WinUI.

For what its worth, game frameworks have been doing fine with float and C#, so this is nothing that can't be solved. The original design of using pretend double and silent truncate is just wrong.

@stevenbrix
Copy link
Contributor

The original design of using pretend double and silent truncate is just wrong.

Agreed, I think these should just project as to how they are implemented under the covers (as floats). If the compiler/runtime need to be fixed to accept floats then in my opinion, that's what we should do. FrameworkElement.Width is a double though, so is this one of those cases where our internal types behave differently than external ones?

@Scottj1s
Copy link
Member

Scottj1s commented Mar 27, 2020

Following a related discussion, the proposal was put forward to maintain the double precision property accessors, say Point.X, and add bounds checking (i.e., throw if the double is larger than a float), but also add direct and efficient access to single-precision backing stores. The projected struct would be blittable, so crossing the ABI would be cheap.

So,
Point pt;
pt.X = GetDouble();
pt.x = GetFloat();

thoughts?

@marcelwgn
Copy link
Contributor

I think that this could potentially create problems of setting the value to a double accidently and applications starting to now having to handle this exception.

On the other hand, that would definitely raise awareness for the situation and people will definitely know when the values are unprecise.

@weltkante
Copy link

throw if the double is larger than a float

What exactly would that mean? Float already includes infinity so I assume that is special cased. Do you mean checking for the absolute value like bounds checks for integer numbers do? That would be equivalent to an upper bound to the exponent. I don't think exceeding that happens very often, it would be more useful to have a lower bounds check for the exponent, i.e. when you get close enough to zero that conversion to float would underflow the exponent.

Event that may not happening very much in practice, what people are probably running into is mantissa overflow, i.e. conversion to float clipping off the mantissa but you needed those bits. Checking that in a sensible way is probably impossible because basically any number not divisible by two will have nonregular bits in the clipped mantissa area. How is the cast going to know if those were important?

@benstevens48
Copy link
Author

Don't throw an exception! I don't see how that's going to help anybody. People won't realise that throwing an exception is possible and the only thing that will do is lead to the possibility of more unhanded exceptions (although it should be rare anyway). I'm also slightly unclear as to what 'larger than a float' means.

Otherwise, that seems reasonable, although I'm not 100% sure about lower case x for the property name. The lower case seems to go against existing C# and UWP naming conventions.

Certainly the existence of this float property would help to flag up that something odd was going on, which might encourage people to check the docs, which would then offer an explanation.

@jtorjo
Copy link

jtorjo commented Mar 28, 2020

@Scottj1s

Following a related discussion, the proposal was put forward to maintain the double precision property accessors, say Point.X, and add bounds checking (i.e., throw if the double is larger than a float), but also add direct and efficient access to single-precision backing stores. The projected struct would be blittable, so crossing the ABI would be cheap.

So,
Point pt;
pt.X = GetDouble();
pt.x = GetFloat();

thoughts?

I think this will cause more confusion in the long run. What I mean is - people expect UpperCamelCase, so propbably 99.9% will use .X regardless.

My point (pun intended) is that this is a class whose usage should be utterly trivial. It's a point with 2 coordinates - X, Y - that's it. Why make it complex?

Offering 'double' projection is a big mistake IMHO:

  1. dealing with (double.)infinity will not work
  2. Because of precision rounding, I believe you can into issues like this
    Point p;
    p.X = some_crazy_double_value;
    if ( p.X == some_crazy_double_value) 
        // will never execute
        do_stuff();

@Scottj1s
Copy link
Member

I appreciate the feedback on this.

We have the option to maintain source compatibility (which I don't think anyone is advocating), with:

struct Point
{
    float _x;
    float _y;

    public Point(double x, double y)
    {
        _x = (float)x;
        _y = (float)y;
    }

    public double X { get => _x; set =>_x = (float)value; }
    public double Y { get => _y; set =>_y = (float)value; }
}

Or validate fields, perhaps with something like this:

struct Point
{
    public Point(float x, float y)
    {
        _x = x; 
        _y = y;
    }

    public Point(double x, double y)
    {
        _x = (float)x; 
        _y = (float)y;
        Validate(_x, x);
        Validate(_y, y);
    }

    public float _x;
    public double X { get => _x; set { _x = (float)value; Validate(_x, value); } }

    public float _y;
    public double Y { get => _y; set { _y = (float)value; Validate(_y, value); } }

    [Conditional("DEBUG")]  // or "WINRT_VALIDATE"
    void Validate(float field, double value, [CallerMemberName] string member = "")
    {
        if ((double)field != value && !float.IsNaN(field))
        {
            throw new ArgumentOutOfRangeException(member);
        }
    }
};

The second version is effectively a superset of the existing Point struct.

Any concerns?

@weltkante
Copy link

weltkante commented May 19, 2020

UWP 10240 was already float for Point members in the API. Only C# saw it as double as far as I understand it. Maybe I'm misunderstanding things, I'm not excluding that.

So its not a change in the WinUI API, its a change in the projection, and if the double illusion is to be perpetuated there the question is at which scope.

@MikeHillberg
Copy link
Contributor

So its not a change in the WinUI API, its a change in the projection,

That's right, that's exactly what it is. The WinRT API was and is single precision, as are the fields in the .Net Native projection. But that projection has double-typed properties that wrap the fields and do the type casting.

So this issue is specifically about the projection. And the preview projection for .Net5 doesn't have the double conversion.

@benstevens48
Copy link
Author

@dotMorten I do agree that it would probably make sense to change all doubles-that-are-really-floats to floats if this happens at all. Is that being considered? In an ideal world I think the best solution would be that all the doubles-that-are really-floats would in fact be doubles-that-are-really-doubles, but that's not going to happen as far as I can see. I no longer know what the best solution is. I think maybe making it very clear in the docs and intellisense that the properties are in fact only single-precision even though they are doubles is the best way forward, along with perhaps a direct access to the floats as described in #2047 (comment). It's important to at least know these properties are really single precision when doing things like coordinate geometry calculations which require an error tolerance.

@stevenbrix
Copy link
Contributor

stevenbrix commented May 19, 2020

I no longer know what the best solution is

Same same :)

I don't think moving all the WinUI type to float is feasible. It would be too large of a breaking change that would make migrating more difficult.

I think it's most important to maintain source compatibility, and the pain this causes folks who want to interop with WPF/UWP is not worth the change.

I like the suggestion in that comment as well, although I'm not really a fan of the XValue and YValue properties. We could provide the constructors as Scott suggested, and have a Roslyn analyzer detect usage of the constructor that takes a double and provides a little lightbulb that says this could truncate. Or even simpler, just have xml comments that would provide Intellisense help text

struct Point
{
    public Point(float x, float y)
    {
        _x = x; 
        _y = y;
    }
   
   /// <summary>
   /// Warning: The values for X and Y could be truncated if you require greater precision due to the backing storage being float.
   /// </summary>
    public Point(double x, double y)
    {
        _x = (float)x; 
        _y = (float)y;
    }

    float _x;
    public double X { get => _x; set { _x = (float)value; } }

    float _y;
    public double Y { get => _y; set { _y = (float)value; } }

}

@dotMorten
Copy link
Contributor

I'm sort of torn now that I'm realizing the WinRT type is float. It's ugly to deal with but I sort of also get it now.

@mdtauk
Copy link
Contributor

mdtauk commented May 19, 2020

If you keep the defaults default for both Win32 and UWP - is there a way to enable developers to override the double and use a float?

@Scottj1s
Copy link
Member

This is a tough call, which is why we're previewing with the switch from double to float, so we can collect community feedback. Regarding converting comprehensively from double to float, it's only an issue for the 3 geometry structs. These were custom WinRT/.NET type mapping conversions. Note that no other custom value type mappings in the link above have float-based fields - they're all double or integral. That includes all the Windows.UI.Xaml* types (e.g, Thickness) and system types like TimeSpan. So the impedance mismatch we need to address is just for these 3 types.

@Scottj1s
Copy link
Member

@mdtauk, yes, we could certainly provide conditional compilation to switch between double and float at the property accessors, constructors

@dotMorten
Copy link
Contributor

dotMorten commented May 20, 2020

I littered the headers of my code with this:

#if WINUI && NETCOREAPP
using uiunits = System.Single; 
#else
using uiunits = System.Double;
#endif

Then use var instead of double to rely on implicit types and generally cast like this:
new Point((uiunits)x, (uiunits)y);

That made the code changes a little more bearable, but it was still over a 100 files I had to touch.

(note most of this code compiles for WPF, UWP and WinUI)

@dotMorten
Copy link
Contributor

dotMorten commented May 20, 2020

Here's an alternative idea: Introduce Microsoft.UI.Point/Rect/Size and use those for all the UI layout stuff instead, and make these double-precision, so we don't have to do all this casting back and forth, and stop using Windows.Foundation.Point/Rect/Size. It seemed weird these windows types are used throughout the layout stuff anyway.

@benstevens48
Copy link
Author

Note that no other custom value type mappings in the link above have float-based fields - they're all double or integral.

@Scottj1s - this may be true in terms of types, but is is true for all properties which are projected as doubles, e.g. ActualHeight, ActualWidth? I haven't tested it, but given that the new ActualSize property is floating-point, I find it difficult to believe that ActualWidth and ActualHeight are really doubles. I suppose this doesn't matter so much since you can't actually set these, but it would still be nice to be consistent.

@Scottj1s
Copy link
Member

@benstevens48 - I was only referring to the switching pain to fix what were formerly projected (incorrectly) as doubles - the geometry types. ActualHeight and ActualWidth may well be stored internally as floats, given ActualSize is a vector2, as you point out. But that's an implementation detail of UIElement/FrameworkElement. These properties are already projected faithfully as doubles.
@dotMorten - I think that might be an even larger breaking change. Would like to hear the WinUI team weigh in on that suggestion.

@michael-hawker
Copy link
Collaborator

🦙 Cross-linking to the new CS/WinRT repo.

@Scottj1s
Copy link
Member

Scottj1s commented Jun 3, 2020

@dotMorten, et al - thanks for the feedback. We've reviewed the switch from double to float properties on Rect, Point, and Size. While it makes the projection technically "correct", it creates too much impedance with other double properties and variables, and it creates major compatibility issues. So we've decided to revert the change.

@benstevens48
Copy link
Author

@Scottj1s - do you plan on exposing direct access to the float properties (via alternative variable names), and maybe a float constructor, as discussed earlier in this thread?

@Scottj1s
Copy link
Member

Scottj1s commented Jun 3, 2020

@benstevens48 - thanks for the reminder. Yes, I'll add float constructors, and I'll also make the backing fields public for these structs. Clearly, this is a special case that warrants bending the usual rules of data hiding, etc.

@benstevens48
Copy link
Author

@Scottj1s - can you consider exposing the backing fields via a property getter/setter that conforms to C# naming conventions?

@stevenbrix
Copy link
Contributor

@benstevens48 what exactly are you trying to accomplish? Is this for use with WinUI, or are you trying to use these types outside of WinUI?

@Scottj1s
Copy link
Member

Scottj1s commented Jun 3, 2020

@benstevens48 - can you provide a specific example, say for Point? Is this for binding purposes?

@benstevens48
Copy link
Author

Actually, maybe it is better just to have the double properties exposed.

My main concern is that it should be clearly documented that the actual stored precision in single. This should include the docs you get on itellisense hover over the field, because sometimes that's all I use. I want to prevent other from making the same mistake I did of using these structures for GPS coordinates and the other issues I mentioned at the very beginning.

My main original use case outside WinUI was for a double-precision point/vector2 (since as of yet .net doesn't have it), but obviously that's actually not possible, although it's so very tempting to use Point for this, hence my plea for clear docs.

The other main use I have is the Rect class with Win2D. Actually I write code in both C# and C++ for this, sometimes copying between, and it's a bit awkward having floats for one and doubles for the other, but exposing the floats wouldn't help that much anyway unless the names were the same, but I'm not sure that exposing names which don't conform to C# conventions is a good idea.

@michael-hawker
Copy link
Collaborator

Since displays are getting bigger, does it just make sense to instead make the backing fields double precision a the API surface is already saying they're as such? Or would there be just as many ramifications of changing that?

@Scottj1s
Copy link
Member

Scottj1s commented Jun 3, 2020

Changing the backing fields is a change to the ABI, with much greater ripples and compat issues, affecting other languages and component implementations that use those types. Changing everything else (constructors, property accessors, methods, etc) only affects C# client code.

@jtorjo
Copy link

jtorjo commented Jun 3, 2020

My main original use case outside WinUI was for a double-precision point/vector2

There are extension methods for that:

image

@benstevens48
Copy link
Author

@jtorjo - I mean I wanted a way of storing a pair of double precision values. Obviously using Point turned out to be wrong, but it took man a while to realize. .Net only has single precision Vector2 currently (although there is a double proposal I think).

@Scottj1s
Copy link
Member

This is one of those design mistakes that turns out be very painful later - for the friction it causes developers either way. Leaving it as is and choosing to fix it both present challenges. At some point, a metadata change might be considered for WinUI (as has been suggested). New interfaces/methods could then be opted into. Meantime, we felt it best to preserve legacy behavior at the projection layer (now provided by C#/WinRT) to avoid surprises and inconsistencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working team-Markup Issue for the Markup team
Projects
None yet
Development

No branches or pull requests