Discrepancy in type and size of parameters during (animation) updates

[ericwj]

Release Type:

• releases/4.1.0.1734-7-gb1a503f2 (master)
• releases/4.1.0.1734-55-g4dc12c95 (#1469)

Platform(s): Windows

Describe the bug The types of equally named parameters are not guaranteed to be equal, but their values are expected to be the same size and have the same meaning for the purpose of updating animations and setting other updatable parameters.

The following tests fail after adding an assertion on the type or the size of equally named parameters.

Test	Parameter Name	Initial Size Type	Updated Size Type	Assertions
Stride.Physics.Tests.SkinnedTest.SkinnedTest1	"Material.SpecularValue"	12 Color3	16 Color4	3
Stride.Graphics.Tests.MaterialTests.MaterialSpecular	"Material.SpecularValue"	12 Color3	16 Color4	1

The parameters may be declared differently than they are used. This should not be possible.

• In these cases, this may lead to loss of opacity since the new (Updated) bytes at offset 12-15 which contain the 4-byte float Color4.Alpha will become zero from the newly allocated byte array into which the old (Initial) values are copied. If the visible color ending up in hardware is the Color4 instead of the Color3, the result is transparent.
• However, if the issue cannot be proven to (just) affect the tests mentioned, there is no telling how serious, wrong or visible this discrepancy in type and/or size could really be.

To Reproduce Steps to reproduce the behavior:

1. Open ParameterCollection.cs
2. Find the text // Second pass to copy existing data at new offsets/slots
3. Paste the following code inside the for loop that starts after this comment
4. Run the tests mentioned and observe them fail.
5. Examine the stacktrace produced and verify that they have ParameterCollection.UpdateLayout just before Debug.Fail
6. Debug the tests and verify that UpdateLayout is in the process of updating the right existing parameter value with the right incoming parameter value (by name) when the check introduced fails

if (parameterKeyInfos[i].Key.PropertyType != newParameterKeyInfos[i].Key.PropertyType)
    Debug.Fail($"{parameterKeyInfos[i].Key.PropertyType} != {newParameterKeyInfos[i].Key.PropertyType}");

Expected behavior The types (and hence) sizes of equally named parameters are equal.

Log and callstacks (line numbers may vary depending on which branch is used, the below is from #1469)

 Stride.Physics.Tests.SkinnedTest.SkinnedTest1
   Source: SkinnedTest.cs line 47
   Duration: 2.8 sec

  Message: 
    Microsoft.VisualStudio.TestPlatform.TestHost.DebugAssertException : Method Debug.Fail failed with 'Stride.Core.Mathematics.Color3 != Stride.Core.Mathematics.Color4
    ', and was translated to Microsoft.VisualStudio.TestPlatform.TestHost.DebugAssertException to avoid terminating the process hosting the test.

  Stack Trace: 
    Debug.Fail(String message)
    ParameterCollection.UpdateLayout(ParameterCollectionLayout collectionLayout) line 635
    Copier.Compile() line 827
    Copier.Copy() line 778
    MaterialRenderFeature.UpdateMaterial(RenderSystem renderSystem, RenderDrawContext context, MaterialInfoBase materialInfo, Int32 materialSlotIndex, RenderEffect renderEffect, ParameterCollection materialParameters) line 370
    <>c__DisplayClass9_0.<Prepare>b__1(Int32 renderNodeIndex, RenderDrawContext threadContext) line 300
    Dispatcher.ExecuteBatch[TLocal](Int32 fromInclusive, Int32 toExclusive, Func`1 initializeLocal, Action`2 action, Action`1 finalizeLocal) line 357
    Dispatcher.Fork[TLocal](Int32 endExclusive, Int32 batchSize, Int32 maxDegreeOfParallelism, Func`1 initializeLocal, Action`2 action, Action`1 finalizeLocal, BatchState state) line 445
    <>c__DisplayClass19_0`1.<Fork>b__0() line 434
    ThreadPool.TryCooperate() line 106
    ThreadPool.WorkerThreadScope() line 137
    Thread.StartCallback()

Additional context Debug assertions where added in the PR mentioned to verify unsafe copy operations. ParameterCollection was failing these assertions, mostly caused by the buffer overrun in CopyTo<T>, which is fixed in the mentioned PR. However after that fix, UpdateLayout kept failing and the reason is the change in size between Color3 and Color4. No other tests start failing after changing the assertion from checking the size to checking the type of the parameters.

• UpdateLayout was modified to copy once if the parameter sizes are equal, but to copy item-for-item if the sizes differ, to force potentially uninitialized bytes to zero.
• A #warning was inserted in UpdateLayout to keep the discrepancy visible when building Stride, pending a fix. The text is "Partially copying parameter values and leaving remaining bytes zero may cause undesired side effects such as e.g. Color4.Alpha becoming zero."

Solution Find out why and how the type discrepancy could arise and remedy the discrepancy systematically. Modify UpdateLayout to not accept type discrepancies any more.

[xen2]

Good catch, will need to find a way to prevent this from happening.