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

Use double for compositor types #11768

Merged
merged 2 commits into from
Jun 16, 2023
Merged

Use double for compositor types #11768

merged 2 commits into from
Jun 16, 2023

Conversation

kekekeks
Copy link
Member

We need to use double values for visuals since we have problems with precision otherwise.

Unfortunately it's a breaking change and we can't keep complete compatibility with RC1. Source code should be compatible with recompilation since implicit conversions from Vector2/Vector3 to Vector/Vector3D were added.

See:
#11641
AvaloniaUI/Avalonia.Controls.TreeDataGrid#176

break;
case RefreshVisualizerState.Pending:
contentVisual.Opacity = 1;
contentVisual.RotationAngle = _startingRotationAngle + (float)(2 * Math.PI);
if (IsPullDirectionVertical)
{
offset = new Vector3(0, (float)(_interactionRatio * (IsPullDirectionFar ? -1 : 1) * root.Bounds.Height), 0);
offset = new Vector3D(0, (float)(_interactionRatio * (IsPullDirectionFar ? -1 : 1) * root.Bounds.Height), 0);
Copy link
Contributor

@Sorien Sorien Jun 13, 2023

Choose a reason for hiding this comment

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

there is still float cast, same for line 270

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0036472-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@DJGosnell
Copy link
Contributor

Just wanted to ask the question since it appears we are effectively replacing floats (4 bytes) for doubles (8 bytes), what is the anticipated impact on performance of this change on desktops along with smaller/embedded devices?

@kekekeks
Copy link
Member Author

Extra 120 bytes per visual

@robloo
Copy link
Contributor

robloo commented Jun 14, 2023

I'm a little-bit confused by this.

  1. How was the graphics-card support problem solved? Several only support float.
  2. The vast majority of core rendering systems use single floating-point precision. This is obvious in even the .NET built-in vector types that support hardware acceleration.

Fundamentally, the rendering system I thought shouldn't care what is outside it's viewport bounds. The layout system should handle all measuring in double precision and by the time it gets to the rendering layer you basically only worry about what is in the viewport. Obviously, what is in the viewport is very small and float is just fine.

I'm worried there is a deeper architectural problem we are missing here. Other systems don't require double in the rendering layer to avoid these issues AFAIK.

Disclaimer: I have little knowledge of this area and the above is just my assumption of how things should work. UWP/WinUI works just fine though with single floating point precision.

@maxkatz6
Copy link
Member

maxkatz6 commented Jun 14, 2023

@robloo
I can't really answer in detail, worth waiting for @kekekeks response. But

How was the graphics-card support problem solved? Several only support float.

It is cast back to float in the Skia code.
The problem was on the Avalonia compositor side which was failing on matrix multiplication with float precision. As on the layout and compositing renderer levels, all of the offsets are preserved. But on the final "draw rectangle" level it's normalized.

UWP/WinUI works just fine though with single floating point precision.

That repro with a button and huge margin doesn't work in UWP and most of frameworks: https://github.com/AvaloniaUI/Avalonia/pull/11768/files#diff-307943b78ddd2a40548b0dc76bfc01a8875b2b505ca85d3007f018faa1f4ba42

Assumingly same would go with enormously huge lists as well, but this one is harder to reproduce on other frameworks.

@maxkatz6
Copy link
Member

Also, Steven's comment about smooth vs logical scrolling should clarify why some controls like DataGrid could handle it with floats #11641 (comment).
In other words, with logical scrolling offset, it uses a virtual scrolling unit equal to the height of a single item, making scrolling experience worse in general. Scroll offset is treated as "offset by 10 items" instead of actual "offset by 1000 pixels" in this case.

@robloo
Copy link
Contributor

robloo commented Jun 14, 2023

Ah, the explanation for DataGrid answers a question how that could work. It seemed like something was still missing there.

It sounds like you have talked this over in detail. Hopefully there are no major performance impacts from the overhead and loss of hardware accelerated types. I'll keep quiet otherwise though.

@kekekeks
Copy link
Member Author

How was the graphics-card support problem solved? Several only support float.

It's about computations that happen before passing any commands to the GPU. I. e. consider the following example:

  <ScrollViewer>
    <StackPanel>
      <Button Margin="0 1000000000 0 0">0</Button>
      <Button>1</Button>
    </StackPanel>
  </ScrollViewer>

Once the ScrollViewer is scrolled down completely the first Button transform would be translate(0, 900) assuming the window height of 1000. That is what will be passed to Skia and in turn to GPU. But on our side we need to use double precision to calculate that final transform matrix correctly. If we use single precision floats the buttons would appear overlayed. The errors can appear with ~8000000 pixel offsets which is just 160K items in a virtualized list. It still happens with double precision floats, but with much larger numbers that aren't likely to appear in apps

Fundamentally, the rendering system I thought shouldn't care what is outside it's viewport bounds. The layout system should handle all measuring in double precision and by the time it gets to the rendering layer you basically only worry about what is in the viewport.

Offsets can be specified outside of the layout system, e. g.

<Border RenderTransform="translate(0, 10000000100)">
  <Border RenderTransform="translate(0, -10000000000)">
    <Button Content="Foo"/>
  </Border>
</Border>

should render the Button at 100px offset, not at 0px offset.

Visual offsets can be animated on the render thread too, so we need to preserve precision there.

You can consider this to be "offloading layout tasks to the render thread" if you want, it's still more efficient than doing such computations on the busy UI thread.

The vast majority of core rendering systems use single floating-point precision. This is obvious in even the .NET built-in vector types that support hardware acceleration.

WPF passes double precision numbers to UCE, so various transform computations happen on the compositor side too. We need compatibility with pre-existing WPF code and concepts, not with "majority of core rendering systems" when it comes to processing the visual tree.

@robloo
Copy link
Contributor

robloo commented Jun 14, 2023

@kekekeks Thanks for the detailed explanation.

Once the ScrollViewer is scrolled down completely the first Button transform would be translate(0, 900) assuming the window height of 1000. That is what will be passed to Skia and in turn to GPU. But on our side we need to use double precision to calculate that final transform matrix correctly

That makes sense to me and is what I would normally expect. Layout calculation must use double precision due to the math and precision. But by the time it gets to Skia and in turn the GPU the range of numbers is much, much smaller.

Offsets can be specified outside of the layout system, e. g.

The RenderTransform is a good example of why not everything can be done in the layout system before rendering. However, requiring bounds less than 10 billion might be perfectly reasonable ;) RenderTransform usage in practice is much smaller than that.

It does sound like layout calculations can be done in a slightly different way (before actual rendering) to avoid this issue. The composition rendering system is really doing the last step of layout itself before handing off to Skia and the GPU. I also suspect WinUI does some tricks here we could learn from in the future (they get a lot wrong too though with Point, Rect, etc. being single precision). But if this strategy worked for WPF I guess there is precedent and if Avalonia's composition renderer needs to support this for WPF compatibility that also makes sense.

@grokys
Copy link
Member

grokys commented Jun 14, 2023

It does sound like layout calculations can be done in a slightly different way (before actual rendering) to avoid this issue.

Layout calculations are done using double and so aren't a problem. The problem is that the renderer has (had) lower precision than the layout system. The reason this shows up now is that our virtualizing panels now use the effective viewport of the scroll area instead of logical scrolling, like ItemsRepeater. This has the advantage that one can e.g. nest or stack ListBoxes and the ones outside the effective viewport won't create items as they can determine that they're not visible. The disadvantage is that their positions are real positions in the viewport, meaning they can be large numbers.

I also suspect WinUI does some tricks here we could learn from in the future

WinUI also has the problem that prompted this change actually. Try creating an ItemsRepeater with many millions of items and you'll see that as you scroll down their positioning starts to break, or try doing something like this:

image

@rabbitism
Copy link
Contributor

Can't believe that listbox performance issue leads to such a big change. 😱

@DJGosnell
Copy link
Contributor

DJGosnell commented Jun 14, 2023

My main concern was this is bringing pretty big memory impacts to all visuals and potentially performance degradations for what I thought was an edge case, but maybe showing millions of items at a time or having visuals offset half a billion units is more common than I thought.

@kekekeks
Copy link
Member Author

Can't believe that listbox performance issue leads to such a big change.

Note that 0.10.x renderers are using double precision. The single precision was introduced as a part of UWP composition API clone.

@grokys grokys added this pull request to the merge queue Jun 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 16, 2023
@maxkatz6 maxkatz6 added this pull request to the merge queue Jun 16, 2023
Merged via the queue into master with commit 4a72036 Jun 16, 2023
@maxkatz6 maxkatz6 deleted the feature/double-composition branch June 16, 2023 22:52
@nhoefer2
Copy link

I just noticed that avalonia has its own Vector3D type. I'm curious if the motivation for this was because system.numerics only supports floats for Vector3. I was looking at this issue here dotnet/runtime#24168 and from I can tell, there are complexities involved due to support for SIMD instructions on double types which can impact performance with hardware acceleration. Does anyone here know if the avalonia Vector3D can take advantage of possible hardware acceleration? From the quick glance I saw in the source, it seems like just an ordinary struct doing the usual mathematical operations that I would expect on a vector type (contrasted with the system.numerics.Vector3 which seems to have a bit more going on).

Please excuse my ignorance on this matter.

@kekekeks
Copy link
Member Author

I'm curious if the motivation for this was because system.numerics only supports floats for Vector3

See #11641 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants