CompositionAnimation fixes#20936
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses broken composition expression animations by simplifying the expression numeric type system (removing the Scalar variant and standardizing on Double), fixing property lookup for server objects during animation evaluation, and centralizing expression keyword strings to reduce duplication and inconsistencies.
Changes:
- Removed
VariantType.Scalarusage paths and updated tests/operators/FFI mappings to useVariantType.Doublefor scalar math. - Reworked server-side composition property lookup to rely on
ServerObject.GetCompositionProperty(...)instead of a global registry/dictionary path. - Introduced
ExpressionKeywordsconstants and updated parsing/reference tracking, including special handling forthis.target.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Avalonia.Base.UnitTests/Composition/CompositionAnimationTests.cs | Updates assertions to validate scalar results via .Double instead of .Scalar. |
| tests/Avalonia.Base.UnitTests/Composition/CompositionAnimationParserTests.cs | Removes Scalar-type branching and expects expression results as Double. |
| src/tools/DevGenerators/CompositionGenerator/Generator.cs | Expands variant-capable property types to include double and Vector. |
| src/Avalonia.Base/Rendering/Composition/Server/ServerObjectAnimations.cs | Switches animation property lookup to GetCompositionProperty on the owner. |
| src/Avalonia.Base/Rendering/Composition/Server/ServerObject.cs | Uses GetCompositionProperty for expression property access when not animated. |
| src/Avalonia.Base/Rendering/Composition/Server/CompositionProperty.cs | Removes the dynamic registry implementation and leaves a simplified registration path. |
| src/Avalonia.Base/Rendering/Composition/Expressions/ExpressionVariant.cs | Removes Scalar representation and refactors operators/casting accordingly. |
| src/Avalonia.Base/Rendering/Composition/Expressions/ExpressionParser.cs | Uses centralized ExpressionKeywords for keyword parsing. |
| src/Avalonia.Base/Rendering/Composition/Expressions/Expression.cs | Adds ExpressionKeywords and updates reference collection for this.target. |
| src/Avalonia.Base/Rendering/Composition/Expressions/DelegateExpressionFfi.cs | Maps float to VariantType.Double and removes Scalar-related casts. |
| src/Avalonia.Base/Rendering/Composition/Animations/AnimationInstanceBase.cs | Tracks this.target references by mapping them to the animation’s TargetObject. |
Comments suppressed due to low confidence (2)
src/Avalonia.Base/Rendering/Composition/Expressions/ExpressionVariant.cs:441
- Now that scalar literals are
VariantType.Double,operator *no longer handles scalar scaling for most vector/matrix/quaternion types. For example,Vector2 * Double/Vector3 * Double/Vector4 * Doublewill currently returndefault(invalid) because onlyVector * Doubleis implemented. Please add the missing* Doublebranches (casting tofloatwhere required) for the types that previously supported* Scalar.
if (left.Type == VariantType.Vector2 && right.Type == VariantType.Vector2)
return left.Vector2 * right.Vector2;
if (left.Type == VariantType.Vector && right.Type == VariantType.Vector)
return Vector.Multiply(left.Vector, right.Vector);
if (left.Type == VariantType.Vector && right.Type == VariantType.Double)
return left.Vector * right.Double;
src/Avalonia.Base/Rendering/Composition/Expressions/DelegateExpressionFfi.cs:118
- Mapping
floattoVariantType.DoublemakesfloatanddoubleFFI overloads indistinguishable at runtime. The registry will then pick the first matching record based on insertion order, and can also force double values to be cast down tofloatinside delegates. This is observable with existing built-ins likeVector2(float,float)andVector2(double,double)/Vector3(float,float,float)andVector3(double,double,double)which will now share the same signature. Consider normalizing built-in scalar signatures todouble(or rejecting duplicate registrations) to avoid order-dependent behavior and precision loss.
static readonly Dictionary<Type, VariantType> TypeMap = new Dictionary<Type, VariantType>
{
[typeof(bool)] = VariantType.Boolean,
[typeof(float)] = VariantType.Double,
[typeof(double)] = VariantType.Double,
[typeof(Vector2)] = VariantType.Vector2,
[typeof(Vector)] = VariantType.Vector,
[typeof(Vector3)] = VariantType.Vector3,
[typeof(Vector3D)] = VariantType.Vector3D,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Avalonia.Base/Rendering/Composition/Expressions/ExpressionVariant.cs
Show resolved
Hide resolved
src/Avalonia.Base/Rendering/Composition/Server/CompositionProperty.cs
Outdated
Show resolved
Hide resolved
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
There was a problem hiding this comment.
Pull request overview
This PR fixes broken Composition/Expression animations by simplifying the expression numeric type system (removing the Scalar variant and standardizing on Double), centralizing expression keyword strings, and adjusting composition property lookup/tracking so expression dependencies and property resolution work correctly.
Changes:
- Remove
VariantType.Scalarand migrate scalar parsing/math/casting toVariantType.Double. - Centralize expression keyword literals via
ExpressionKeywordsand update parsing/reference collection/tracking (includingthis.Target). - Switch composition property lookup to
ServerObject.GetCompositionProperty, update generator type support (double,Vector), and add/adjust unit tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Avalonia.Base.UnitTests/Composition/CompositionAnimationTests.cs | Updates scalar assertions to Double and adds new coverage for property lookup + expression tracking. |
| tests/Avalonia.Base.UnitTests/Composition/CompositionAnimationParserTests.cs | Updates parser test to expect only Double results for scalars. |
| src/tools/DevGenerators/CompositionGenerator/Generator.cs | Extends variant get-property generation to include double and Vector. |
| src/Avalonia.Base/Rendering/Composition/Server/ServerObjectAnimations.cs | Uses GetCompositionProperty for animation property resolution; improves subscription invalidation behavior. |
| src/Avalonia.Base/Rendering/Composition/Server/ServerObject.cs | Routes expression property access through GetCompositionProperty. |
| src/Avalonia.Base/Rendering/Composition/Server/CompositionProperty.cs | Removes the old global per-type registry mechanics, keeping property creation only. |
| src/Avalonia.Base/Rendering/Composition/Expressions/ExpressionVariant.cs | Removes Scalar, updates arithmetic/comparison/casting, and standardizes scalar behavior on Double. |
| src/Avalonia.Base/Rendering/Composition/Expressions/ExpressionParser.cs | Replaces keyword literals with ExpressionKeywords. |
| src/Avalonia.Base/Rendering/Composition/Expressions/Expression.cs | Introduces ExpressionKeywords and updates reference collection for this.Target. |
| src/Avalonia.Base/Rendering/Composition/Expressions/DelegateExpressionFfi.cs | Updates type mapping (float → Double) and removes Scalar cast logic. |
| src/Avalonia.Base/Rendering/Composition/Animations/AnimationInstanceBase.cs | Special-cases reference tracking for this.Target so target object dependencies are subscribed correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Avalonia.Base/Rendering/Composition/Expressions/ExpressionVariant.cs
Outdated
Show resolved
Hide resolved
tests/Avalonia.Base.UnitTests/Composition/CompositionAnimationTests.cs
Outdated
Show resolved
Hide resolved
tests/Avalonia.Base.UnitTests/Composition/CompositionAnimationTests.cs
Outdated
Show resolved
Hide resolved
tests/Avalonia.Base.UnitTests/Composition/CompositionAnimationTests.cs
Outdated
Show resolved
Hide resolved
src/Avalonia.Base/Rendering/Composition/Expressions/Expression.cs
Outdated
Show resolved
Hide resolved
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
There was a problem hiding this comment.
Pull request overview
This PR fixes and refactors Avalonia’s composition animation/expression system by simplifying server-side composition property lookup, standardizing expression keywords, and improving numeric/type handling to restore ExpressionAnimation correctness (including re-queuing during evaluation).
Changes:
- Replaced
CompositionProperty’s global registry-based lookup with per-typeServerObject.GetCompositionProperty(...)lookups and updated animation property access paths accordingly. - Standardized expression keyword strings via
ExpressionKeywordsand improved tracking ofthis.Targetreferences for invalidation/subscription. - Removed
VariantType.Scalarin favor ofVariantType.Doublefor scalar numeric evaluation and expanded generator/type support, with added unit tests for key scenarios.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Avalonia.Base.UnitTests/Composition/CompositionAnimationTests.cs | Adds/updates tests for property lookup, expression operations, reference/target tracking, and re-queuing behavior. |
| tests/Avalonia.Base.UnitTests/Composition/CompositionAnimationParserTests.cs | Updates parser test expectations to the new scalar-as-Double behavior. |
| src/tools/DevGenerators/CompositionGenerator/Generator.cs | Extends generated variant/property support to include double and Vector. |
| src/Avalonia.Base/Rendering/Composition/Server/ServerObjectAnimations.cs | Switches property lookup to _owner.GetCompositionProperty, fixes subscription validity defaults, and ensures direct-value invalidation after animation updates. |
| src/Avalonia.Base/Rendering/Composition/Server/ServerObject.cs | Uses GetCompositionProperty for expression property reads when animations aren’t instantiated. |
| src/Avalonia.Base/Rendering/Composition/Server/ServerCompositorAnimations.cs | Fixes dirty-queue handling so objects can be re-queued during evaluation. |
| src/Avalonia.Base/Rendering/Composition/Server/CompositionProperty.cs | Removes the static registry implementation details from CompositionProperty. |
| src/Avalonia.Base/Rendering/Composition/Expressions/ExpressionVariant.cs | Removes Scalar, updates member access comparisons, revises arithmetic/casting, and refactors TryCast/Create/ToString. |
| src/Avalonia.Base/Rendering/Composition/Expressions/ExpressionParser.cs | Replaces inline keyword strings with centralized ExpressionKeywords constants. |
| src/Avalonia.Base/Rendering/Composition/Expressions/Expression.cs | Introduces ExpressionKeywords and ensures this.Target member access participates in dependency tracking. |
| src/Avalonia.Base/Rendering/Composition/Expressions/DelegateExpressionFfi.cs | Updates FFI type mapping so float is treated as Double for expression evaluation consistency. |
| src/Avalonia.Base/Rendering/Composition/Animations/AnimationInstanceBase.cs | Ensures tracked object subscription resolves this.Target to TargetObject during initialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public bool TryCast<T>(out T res) where T : struct | ||
| { | ||
| if (typeof(T) == typeof(bool)) | ||
| { | ||
| if (Type == VariantType.Boolean) | ||
| { | ||
| res = (T) (object) Boolean; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (typeof(T) == typeof(float)) | ||
| switch (default(T)) | ||
| { |
There was a problem hiding this comment.
ExpressionVariant.Or (just above this block) returns Boolean && right.Boolean, which makes || expressions behave like &&. Since BinaryExpression evaluates ExpressionType.LogicalOr via left.Or(right), this should use logical OR (||).
| } | ||
| => default(T) switch | ||
| { | ||
| bool => (bool)(object)v, |
There was a problem hiding this comment.
ExpressionVariant.Create<T> has cases for float and various structs but no double case, even though VariantType.Double exists and TryCast<T> supports double. Calling Create<double>(...) will currently hit the default arm and throw. Add an explicit double case that creates a VariantType.Double variant.
| bool => (bool)(object)v, | |
| bool => (bool)(object)v, | |
| double => (double)(object)v, |
| System.Numerics.Matrix3x2 => (Matrix3x2)(object)v, | ||
| Avalonia.Matrix _ => (Matrix)(object)v, | ||
| System.Numerics.Matrix4x4 => (Matrix4x4)(object)v, | ||
| System.Numerics.Quaternion => (Quaternion)(object)v, | ||
| Avalonia.Media.Color => (Avalonia.Media.Color)(object)v, |
There was a problem hiding this comment.
ExpressionVariant.Create<T> includes a case for Avalonia.Matrix, but the implicit conversion ExpressionVariant(Matrix value) currently sets Type = VariantType.Matrix3x2 while writing to AvaloniaMatrix. This produces an inconsistent variant (wrong Type) and will break casting/operations on Avalonia.Matrix. Update the implicit conversion to use VariantType.AvaloniaMatrix (and keep Create aligned with that).
|
You can test this PR using the following package version. |
What does the pull request do?
This pull request makes several improvements and fixes to the composition animation and expression system in Avalonia, focusing on simplifying property lookup, improving keyword handling, and enhancing type consistency and test coverage. The most significant changes include refactoring how composition properties are registered and accessed, consolidating expression keywords, and updating type handling for animation expressions.
Refactoring and Simplification of Composition Property Lookup:
CompositionProperty, switching to a simpler property lookup mechanism viaGetCompositionProperty, which is now used throughout the codebase. This reduces complexity and potential threading issues. [1] [2] [3] [4]Expression Keyword Handling Improvements:
ExpressionKeywordsstatic class to centralize and standardize all expression keyword strings, and updated all usages to reference this class. This reduces duplication and the risk of typos. [1] [2] [3] [4]Type System and Animation Expression Updates:
floatfromVariantType.ScalartoVariantType.Doublein expression evaluation, and updated casting logic to reflect this, ensuring more consistent handling of numeric types in animations. [1] [2] [3] [4]Bug Fixes and Internal Improvements:
ServerObjectAnimationsare always marked valid by default, preventing potential null reference issues. [1] [2]What is the current behavior?
Nothing is written to s_dynamicRegistry(CompositionProperty.cs), thus all composition properties seem not correctly registered. Thus, GetPropertyAnimation method(ServerObjectAnimation.cs) always return default, making ExpressionAnimation completely broken.
VariantPropertyTypes (Generator.cs) does not contain Vector type, so it always generate null for GetVariant parameter .
Constants like '0.5' are parsed as Scalar, and when we do operations with double values(e.g. Vector3D.X), it will return Invalid value as they're different ExpressionVarient types.
When another animation is invalidated during an animation's evaluation, the target object is not requeued and the animation won't be evaluated again.
string.Internbehaves differently in AOT, makingGetPropertyfailed (string.Intern works inconsistently with NativeAOT dotnet/dotnet-api-docs#12447)These problems made CompositionAnimation (especially ExpressionAnimation) hard to work properly.
What is the updated/expected behavior with this PR?
20260319-1307-42.2557921.mp4
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
dotnet/runtime#16774
dotnet/runtime#18784