Skip to content

Conversation

PaulDemeulenaere
Copy link
Contributor

@PaulDemeulenaere PaulDemeulenaere commented Dec 1, 2021


Purpose of this PR

Fix instability leading to a timeout in VFXGUITest, the number of listed block in InitializeContext increased from 348 to 1068.
It leads to exploding test time, and sometimes a time out on Yamato.

This PR doesn't resolve the performance issue but is only splitting the CreateAllBlocksTest test in several batch of 256 blocks.

Bonus:

  • The block test on output was trying to add block in VFXStaticMeshOutput (the first listed of modelType VFXContextType.Output), I forced to VFXPlanarPrimitiveOutput to actually cover this behavior.
  • Use of new temporary graph VFXTestCommon.MakeTemporaryGraph API instead of legacy set of GUITest asset (we were writing several time on the same asset before)
  • Avoid local storage of m_ViewController, it made the intialization difficult to follow.
  • Timing experiment is available with ExperimentCreateAllBlocksTiming (disabled, it isn't an actual test)

Testing status

Locally 🟢 (and the split of CreateAllBlocksTest make it faster to run, ~300s to ~120s, not ideal neither)
Yamato 🟢 (always)
image


Comments to reviewers

See also this Conversation & Quick performance analysis

Use common helper to avoid conflict writing twice on the same VFXAsset
- Split several batches for block test
- Avoid storage of m_ViewController (it makes the test complex to follow)
- Avoid closing VFXViewWindow between each test (it changes focus)
- Fix incorrect OutputTest : it wasn't testing anything since the first output is a StaticMeshOutput (which doesn't allow any VFXBlock))
ExperimentCreateAllBlocksTiming(True,True) (524.566s)
---
ApplyChange : True - Blocks
2;366.6056
4;280.8626
8;438.5445
16;916.79
32;2161.929
64;5800.3852
128;19399.343
256;81146.7971
384;144797.5553
512;259999.4175

ExperimentCreateAllBlocksTiming(True,False) (150.551s)
---
ApplyChange : True - Operators
2;195.8352
4;262.6614
8;380.9485
16;546.6944
32;1052.9504
64;2302.2815
128;5838.3726
256;18521.1908
384;39612.1396
512;73306.4304

ApplyChange : False - Blocks
2;158.5336
4;170.5459
8;177.3936
16;171.1138
32;205.7508
64;303.08
128;512.1027
256;1204.1919
384;2270.6415
512;3770.7946

ExperimentCreateAllBlocksTiming(False,False) (12.354s)
---
ApplyChange : False - Operators
2;146.4963
4;147.5193
8;154.3424
16;170.8592
32;216.6918
64;311.6344
128;564.5528
256;1327.3833
384;2454.0963
512;3980.0222
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Dec 1, 2021
Copy link
Contributor

@julienamsellem julienamsellem left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of tests 👍

PaulDemeulenaere added a commit that referenced this pull request Dec 1, 2021
It fixes test in edit mode
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Dec 1, 2021
@PaulDemeulenaere
Copy link
Contributor Author

The issue about VFX_URP isn't related to this change, see this conversation

@PaulDemeulenaere PaulDemeulenaere merged commit b900a89 into master Dec 2, 2021
@PaulDemeulenaere PaulDemeulenaere deleted the vfx/fix/instability-gui-test branch December 2, 2021 08:49
PaulDemeulenaere added a commit that referenced this pull request Dec 16, 2021
* [VFX] Staging to master (#6400)

* Early backport of #6438

It fixes test in edit mode
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.

2 participants