[WinUI] Optimize TransformationExtensions
#22481
Conversation
|
Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
| frameworkElement.RenderTransformOrigin = new global::Windows.Foundation.Point(anchorX, anchorY); | ||
| frameworkElement.RenderTransform = new ScaleTransform { ScaleX = scaleX, ScaleY = scaleY }; |
There was a problem hiding this comment.
Basically this is set to be unset on L27 and L28.
Additionaly, my best guess is that unsetting RenderTransformOrigin is not necessary if RenderTransform is not null.
| if (rotationX % 360 == 0 && rotationY % 360 == 0 && rotation % 360 == 0 && | ||
| translationX == 0 && translationY == 0 && scaleX == 1 && scaleY == 1) |
There was a problem hiding this comment.
If these are all default, this if should be true.
But are there cases were we need to round? What if someone did some math in their code and rotationX is 359.99999999?
There was a problem hiding this comment.
Good point.
I'm not too sure what conventions MAUI or graphics libraries have in this regard. Naively I would say that 359.999999 is not really a big issue because it probably happens quite rarely and function-wise the result will be acceptable (yet slower in performance).
If you have a specific suggestion, please suggest it here. Otherwise, I would like to avoid mixing multiple things in one PR (because it can add months to get this merged). This PR is mostly meant for elements that does no transformations at all (my use case) and I believe it's very very common.
There was a problem hiding this comment.
Maybe someone like @PureWeen or @mattleibow know of an example where we round on other platforms.
I imagine an extension method for double like: IsNearly(0, translationX) or IsNearly(1, scaleX) if we already do this somewhere.
There was a problem hiding this comment.
I imagine an extension method for
doublelike:IsNearly(0, translationX)orIsNearly(1, scaleX)if we already do this somewhere.
Sure, I understand the idea. I'm just not sure how exactly to implement it so that it can be considered "always correct" (like how many decimals, etc.)
There was a problem hiding this comment.
I filed #22513 for this.
MartyIX
force-pushed
the
feature/2024-05-17-TransformationExtensions-Optimization-FINAL
branch
from
93cbade
to
2344995
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@rmarinho Could you take a look please? |
MartyIX
added
the
t/perf
|
@Eilon I would say that My test app does not contain any animation whatsoever, it just contains a biggish grid. So this PR should help everyone who does not use animations. edit: Improved OP. |
Ah, OK. If you know a better area can you suggest/update it? Otherwise I think |
|
I added the t/perf label earlier. Now it's OK, I think. |
MartyIX
deleted the
feature/2024-05-17-TransformationExtensions-Optimization-FINAL
branch
Description of Change
This PR makes it so that
RenderTransformproperty is not set unnecessarily just to be unset immediately after. SettingRenderTransformseems very costly.All UI elements that do not need
RenderTransformwill benefit from this change. For example, creating biggish grids or more complex apps where inefficiencies add up.Speedscope
1st measurement
-> 75% improvement
I need to double check the results because it seems "too much".
2nd measurement
-> 87% improvement
Issues Fixed
Contributes to #21787