-
Notifications
You must be signed in to change notification settings - Fork 660
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
Comments
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 |
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:
So my guess is that, in order to support using points in XAML, they defined them as double in the abstraction layer. |
@stevenbrix will this be resolved by the new projection work? |
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. |
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 |
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 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.) |
@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? |
That would be awesome, a lot of possible truncation bugs will go away. |
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: |
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. |
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
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.) |
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. |
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. |
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. |
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, thoughts? |
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. |
What exactly would that mean? Float already includes 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? |
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 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. |
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:
|
I appreciate the feedback on this. We have the option to maintain source compatibility (which I don't think anyone is advocating), with:
Or validate fields, perhaps with something like this:
The second version is effectively a superset of the existing Point struct. Any concerns? |
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. |
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. |
@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. |
Same same :) I don't think moving all the WinUI type to 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 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; } }
} |
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. |
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? |
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. |
@mdtauk, yes, we could certainly provide conditional compilation to switch between double and float at the property accessors, constructors |
I littered the headers of my code with this:
Then use 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) |
Here's an alternative idea: Introduce |
@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. |
@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. |
🦙 Cross-linking to the new CS/WinRT repo. |
@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. |
@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? |
@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. |
@Scottj1s - can you consider exposing the backing fields via a property getter/setter that conforms to C# naming conventions? |
@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? |
@benstevens48 - can you provide a specific example, say for Point? Is this for binding purposes? |
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. |
Since displays are getting bigger, does it just make sense to instead make the backing fields |
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 - 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). |
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. |
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.
It prints
(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).
The text was updated successfully, but these errors were encountered: