dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.07k stars 1.17k forks source link

Problem with WPF's Slider style (and a few thoughts on porting DX11 rendering engine to Core3) #521

Closed ab4d closed 5 years ago

ab4d commented 5 years ago

Issue Title

WPF's Slider control is not correctly rendered - probably because of a problem with its style.

General

If the following xaml is used to define the slider: <Slider Minimum="0" Maximum="5" Value="4" IsSnapToTickEnabled="True" TickFrequency="1" TickPlacement="BottomRight" Width="100" HorizontalAlignment="Left" VerticalAlignment="Top" />

Then the shown slider is cut on the right side and does not align with the ticks (the same happens when setting TickPlacement to Both):

image

This happens on a screen without any DPI scale (I was not able to try it on high DPI screen). This issue exists from the preview1 version.

Otherwise, I would like to congratulate you on a great job! I have ported my DirectX 11 rendering engine (Ab3d.DXEngine) and WPF 3D toolkit (Ab3d.PowerToys) to .Net Core 3 and they work without any problems from Preview1. Except changing the csproj files there were no changes in the code needed. The code is quite complex and uses SharpDX to call DirectX 11 API and it runs without any problems. I also tested the WPF 3D part and this also runs very well. One of the first things that I did, was to check the performance. I have expected a slight performance improvement, but just recompiling the code for .Net Core 3 brings no performance benefits to my rendering engine. Anyway, in the future, I plan to use new options like Slice and SIMD vectors and this should have more impact.

karelz commented 5 years ago

Thanks for the feedback, it is really great to hear that your migration to .NET Core was smooth!

Is the bug above .NET Core specific? (i.e. It does not happen on .NET Framework.)

ab4d commented 5 years ago

Yes, Slider control is correctly rendered in .Net Framework (from 3.5 to 4.7.2): image

karelz commented 5 years ago

Thanks for confirmation @ab4d!

Berrysoft commented 5 years ago

It is correctly rendered in .Net Framework 4.8, too.

And it is NOT correctly rendered in .Net Core 3.0 with DPI scale, either.

KodamaSakuno commented 5 years ago

Here's the origin of this problem.

double viewportSize = Math.Max(0.0, ViewportSize);

// If viewport is NaN, compute thumb's size based on its desired size,
// otherwise compute the thumb base on the viewport and extent properties
if (double.IsNaN(viewportSize))
{
    ComputeSliderLengths(arrangeSize, isVertical, out decreaseButtonLength, out thumbLength, out increaseButtonLength);
}
else
{
    // Don't arrange if there's not enough content or the track is too small
    if (!ComputeScrollBarLengths(arrangeSize, viewportSize, isVertical, out decreaseButtonLength, out thumbLength, out increaseButtonLength))
    {
        return arrangeSize;
    }
}

Source: https://referencesource.microsoft.com/#PresentationFramework/src/Framework/System/Windows/Controls/Primitives/Track.cs,484

The ViewportSize property is always NaN when the thumb is in a Slider. However, the logic of Math.Max has been changed to match MSVC (https://github.com/dotnet/coreclr/pull/20912), so viewportSize is 0 and the program goes to ComputeScrollBarLengths method. As the result, the thumb is arranged with a wrong size.

UPDATE: Replace decompiled code with the one from Reference Source.

karelz commented 5 years ago

FYI @tannergooding - fallout from some of our alignments in Math area ...

karelz commented 5 years ago

@KodamaSakuno thanks for root causing it! Do you know where in the code it lives? Is that open source yet?

tannergooding commented 5 years ago

However, the logic of Math.Max has been changed to match MSVC (dotnet/coreclr#20912)

Correct. This was updated to be IEEE 754:2008 compliant and to match the behavior defined by the C/C++ language standards in Annex F - IEC 60559 floating-point arithmetic (which all of MSVC, GCC, and Clang currently follow for their implementations).

However, it is worth noting that IEEE 754:2019 (which is currently in draft form) looks to be changing this slightly. Namely, they are removing minNum/maxNum from the list of "required" operations and replacing them with new minimum/maximum and minimumNumber/maximumNumber operations which are recommended by not required and which more clearly specify various behaviors.

Once the IEEE 754 draft is officially published, there will likely be proposals going up to implement the new recommended operations; which would involve exposing the new minimum/maximum functions under System.Math/System.MathF. The names under which they would be exposed likely need more thought.

KodamaSakuno commented 5 years ago

@karelz I got it with a decompiler.

UPDATE: I replaced it with the one from Reference Source.

tannergooding commented 5 years ago

I've opened https://github.com/dotnet/corefx/issues/36925 proposing we partially revert the behavior of Math.Min/Math.Max and expose new functions for the non-propagating functionality.

Ultimately, this would mean we keep the change that considers +0 to be gerater than -0 (for the purposes of these functions only); but we continue propagating NaN so that we don't break people dependent on that behavior. We then provide new functions that don't propagate for people who need/prefer that behavior.

tannergooding commented 5 years ago

This should be unblocked now, once WPF can consume a new coreclr build.

jongfeel commented 5 years ago

Still reproduce at .NET Core SDK 3.0.100-preview4-011223 (Released 2019-04-18) I hope fix it next .NET Core release.

dotnetcore_wpf_slider_thumb_bug

karelz commented 5 years ago

@jongfeel if you follow the links, you can see that the fix is in Math in https://github.com/dotnet/coreclr/pull/24039, commit https://github.com/dotnet/coreclr/commit/c5b2e71106b1a8d2480e1f08bffcb09358f0a45a. It did not get into Preview 4, but it should be part of Preview 5.

miguep commented 5 years ago

This issue has been verified fixed in Preview6. Both by the change made in coreclr and by a change in WPF to remove the dependency on Math.Max/Min NaN behavior.

https://github.com/dotnet/wpf/blob/3366153dd8007ec8984c895093b0d19c1fb4a412/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/Primitives/Track.cs#L486