Fixed a random failure in the DynamicMaterialTest script.

There were sporadic screenshot failures where the colors would be incorrect because the screenshot was taking on seemingly the incorrect frame. In my testing, it failed about 30-40% of the time. This failure started happening at commit debdc13182675e76dc83c3dac3443537671471a2 where AtomSampleViewer's shader variant list for StandardPbr was updated to include all known materials in ASV. This changed provided a specialized shader variant for the material configuration being used. When the test would run, the ShaderVariantAsyncLoader would detect the presence of the shader variant, load it, and trigger the Material to reinitialize itself via Material::OnShaderVariantReinitialized. Sometimes this occured on the same frame as when the DynamicMaterialTest script paused the sample, and the screenshot was also requested on that same frame. Material::Init called Material::Compile() as normal. But this prevented DynamicMaterialTestComponent from compiling material color changes for that frame (because Material::Compile only works once per frame). Since the screenshot was also captured for that frame, it was incorrectly capturing the material colors for the previous frame.

There are two changes required for to fix this. DynamicMaterialTestComponent will try to compile the materials even on frames when material properties were not changed, just in case there were changes on the prior frame that haven't been compiled yet. This ensures that even though the sample is "paused" it will still apply the last change that occurred before the pause. Also DynamicMaterialTest.bv.lua will now sleep for 1 frame after pausing and before taking the screenshot. This one frame delay gives extra time for the delayed Material::Compile operation.

In an upcoming PR hope to improve the Material system to automatically compile any property changes instead of requiring client code to handle this complexity. At that time, I expect DynamicMaterialTestComponent will no longer have to compile materials, and we can hopefully remove the extra sleeps from the script as well.

Testing: Ran the DynamicMaterialTest script repeatedly. Before the fix, it failed 30-40% of the time, with no more than 4 subsequent passing tests. After the fix, I ran the script 25 times without failing and then stopped.

Signed-off-by: santorac <[email protected]>

Gem/Code/Source/DynamicMaterialTestComponent.cpp

@@ -302,7 +302,11 @@ namespace AtomSampleViewer
         if (updateMaterials)
         {
             m_materialConfigs[m_currentMaterialConfig].m_updateLatticeMaterials();
-            CompileMaterials();
         }
+
+        // Even if materials weren't changed on this frame, they still might need to be compiled to apply changes
+        // from a previous frame. This could be the result of a material that was just loaded or reinitialized on
+        // the previous frame, possibly caused by a shader variant hot-loading.
+        CompileMaterials();
     }
 } // namespace AtomSampleViewer

Scripts/DynamicMaterialTest.bv.lua

@@ -31,18 +31,21 @@ function TakeScreenshotSeries(filenamePrefix)
     SelectImageComparisonToleranceLevel("Level H")
 
     -- There could be variation in how long prior activities took so reset the clock for each series of screenshots
-    SetImguiValue('Reset Clock', true) 
-    IdleFrames(1) -- Give time for the sample to make any material changes before starting screenshots.
+    SetImguiValue('Reset Clock', true)     
+    IdleFrames(1) -- Consume ImGui changes, to ensure the clock is reset before pausing
 
     -- Note we pause while taking the screenshot so that IO delays won't impact the timing of the sample
 
     SetImguiValue("Pause", true)
+    IdleFrames(1) -- Give extra time to make sure any material changes are applied, especially in case an asset hot-load causes the material to reinitialize itself.
     CaptureScreenshot(g_screenshotOutputFolder .. filenamePrefix .. '_A.png')
     SetImguiValue("Pause", false)
 
+    -- Let the animation run for 1 second
     IdleSeconds(1.0)
 
     SetImguiValue("Pause", true)
+    IdleFrames(1) -- Give extra time to make sure any material changes are applied, especially in case an asset hot-load causes the material to reinitialize itself.
     CaptureScreenshot(g_screenshotOutputFolder .. filenamePrefix .. '_B.png')
     SetImguiValue("Pause", false)
 end