[VFX] Fix Sanitize behavior #4971

PaulDemeulenaere · 2021-06-24T14:12:00Z

Purpose of this PR

Fix this fogbugz : https://fogbugz.unity3d.com/f/cases/1344645/

VisualEffectGraph importer was using an undefined behavior creating Scriptable object in OnPreprocessAsset.
Move the sanitize to OnPostprocessAllAssets.

Testing status

Added a new editor test which uses the "GetSpawnCount" sanitization as reference, it checks the sanitization of this graph is this one ⬇️

@Unity-Technologies/gfx-qa-vfx With this PR, we are modifying the asset import process, it can have unexpected side effect.
We already have some known issue, it's better to check this change doesn't make them worst :

⚠️ You have to test this change with 21.2b1 or 22.1 or greater (the root change landed in trunk at 2021.2.0b1.3026_53e4e46f810)

Comments to reviewers

See this conversation : https://unity.slack.com/archives/C47JPNNTZ/p1624277477261600?thread_ts=1623913383.239100&cid=C47JPNNTZ

This change of behavior has been introduced by this change : https://ono.unity3d.com/unity/unity/pull-request/126367/_/assetdatabase/mainobject/loaded-asset-management which now deletes object created during OnPreprocessAsset

I have some concerns :

No idea about side effects of this solution, the change risk is pretty high
~~Nothing to prevent compilation before sanitize~~ Fix at cbfa99c
~~It will lead to a double compilation of sanitized files (if there is a sanitize needed, we will have a new reimport)~~ Only once now.
OnPostprocessAllAssets can't be filtered efficiently on the VisualEffectAsset
VFXExternalShaderProcessor also uses OnPreprocessAsset, I didn't check this behavior

N.B.: The change in HDRPProjectSettings.asset is about an unrelated issue with HDWizard which affects editor test (already shared with @RSlysz See this conversation)

See https://unity.slack.com/archives/G1BTWN88Z/p1623825854027500

See this conversation : https://unity.slack.com/archives/C47JPNNTZ/p1624277477261600?thread_ts=1623913383.239100&cid=C47JPNNTZ However, some concerns : - No idea about side effects of this solution - It will lead to a double compilation of sanitized files (if there is a sanitize needed, we will have a new reimport) - OnPostprocessAllAssets can't be filtered efficiently on the VisualEffectAsset

Hoping it will prevent this issue : ``` Unhandled log message: '[Exception] UnityException: DestroyImmediate is not allowed to be called from a ScriptableObject constructor (or instance field initializer), call it in OnEnable instead. Called from ScriptableObject 'HDWizard'. See "Script Serialization" page in the Unity Manual for further details.'. Use UnityEngine.TestTools.LogAssert.Expect ``` See https://yamato-artifactviewer.prd.cds.internal.unity3d.com/9f8a60fd-d1f9-48b5-9513-e4e535cf5d7c%2Flogs%2FTestProjects%2FVisualEffectGraph_HDRP%2Ftest-results/TestReportV1.html

This reverts commit 99ab5fb.

Use a static cache of already santized object This solution remains a pretty dirty trick :-/ *Before* - Preprocess : Compile (possibly with side effect modifying the graph) - PostProcess : Sanitize, if there is a modification, it should relaunch the import & recompile *After* - Preprocess : Compile only if the sanitize has been done - PosProcess : Sanitize, if it's the first sanitize, relaunch the reimport Locally, it fixes unexpecting missing import of point cache asset (see 09_AttributeMaps).

@RSlysz

This instance singleton can be called from HDWizard constructor making this call illegal. See https://unity.slack.com/archives/GHD5LADU7/p1625040944440100?thread_ts=1624547150.387900&cid=GHD5LADU7 (cc @RSlysz) Testing yamato ⏳

This reverts commit 508d713.

It prevents Wizard Popup to be shown while runnning editor test See : https://unity.slack.com/archives/GHD5LADU7/p1625056568458400?thread_ts=1624547150.387900&cid=GHD5LADU7

This reverts commit ff51dcf.

Prevent exception while launching test

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs

- Use m_GraphSanitized instead of an HashSet - ClearRuntimeData when not sanitized (prevent any unexpected call to OnSetupMaterial)

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs

There was an unexpercted custom implementation of MakeTemporaryGraph Use common code : VFXTestCommon.MakeTemporaryGraph()

Fix this issue : #4971 (comment) See : See : https://unity.slack.com/archives/C47JPNNTZ/p1625218128286700?thread_ts=1623913383.239100&cid=C47JPNNTZ

TestProjects/VisualEffectGraph_HDRP/Assets/AllTests/Editor/Tests/VFXDataTests.cs

PaulDemeulenaere · 2021-07-05T06:59:39Z

I also run Yamato at this revision : https://ono.unity3d.com/unity/unity/changeset/4c810dd7b51895691168a6d4b2dff8252ab53311 to anticipate changes from AssetDB about PosProcessAllAsset.
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/vfx%252Ffix%252F1344645-sanitize-failure/.yamato%252Fall-vfx_hdrp.yml%2523Extra_VFX_HDRP_CUSTOM-REVISION/7517180/job
🟢

julienf-unity

Looks like a good workaround. @VitaSkruibyte This is critical changes (as everything touching import workflow) and requires quite broad testing

com.unity.visualeffectgraph/CHANGELOG.md

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs

julienf-unity · 2021-07-06T07:33:32Z

How bad does the double import increase iteration times?

GetOrCreateGraph is also assigning the visualEffectResource to the current graph https://github.com/Unity-Technologies/Graphics/blob/d6a59150d20762251cc5fcb8515fee494a142ccc/com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs#L231 It's needed to correctly update child dependencies This behavior is counterintuitive

PaulDemeulenaere · 2021-07-09T10:12:30Z

The formatting issue is unrelated to this change and has been already be fixed at : 457cf2e

VladNeykov

Tested the VFX Samples as well as a dependency-heavy import/export scenario with subgraphs, nested VFXs, and old/new ShaderGraph integrations. No issues found.

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs

I noticed this issue working on this PR : #5104

# Conflicts: # com.unity.visualeffectgraph/CHANGELOG.md

github-actions · 2021-07-13T07:57:27Z

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

VFX
/.yamato%252Fall-vfx.yml%2523PR_VFX_2021.2

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

VitaVFX

Looks good, couldn't find any breaking changes during testing. Here's the test doc for detailed information on what was tested.

PaulDemeulenaere · 2021-07-13T09:17:55Z

I'm working on the subgraph issue in another PR : #5104

@RSlysz

* Fix case 1346650 where OnGUI draws were overwritten (#5107) * Fix case 1346650 where OnGUI draws were overwritten * update changelog * change conditional check * Revert "Color field keeps updating when using eye dropper and Esc key is pressed (#4950)" (#5122) This reverts commit 587dc15. * [VFX] Fix Sanitize behavior (#4971) * Add regression test on basic sanitize See https://unity.slack.com/archives/G1BTWN88Z/p1623825854027500 * Move SanitizeForImport See this conversation : https://unity.slack.com/archives/C47JPNNTZ/p1624277477261600?thread_ts=1623913383.239100&cid=C47JPNNTZ However, some concerns : - No idea about side effects of this solution - It will lead to a double compilation of sanitized files (if there is a sanitize needed, we will have a new reimport) - OnPostprocessAllAssets can't be filtered efficiently on the VisualEffectAsset * *Minor : Missing InvariantCultureIgnoreCase flag in EndsWith * *Update changelog.md * *Fix HDRP warning Hoping it will prevent this issue : ``` Unhandled log message: '[Exception] UnityException: DestroyImmediate is not allowed to be called from a ScriptableObject constructor (or instance field initializer), call it in OnEnable instead. Called from ScriptableObject 'HDWizard'. See "Script Serialization" page in the Unity Manual for further details.'. Use UnityEngine.TestTools.LogAssert.Expect ``` See https://yamato-artifactviewer.prd.cds.internal.unity3d.com/9f8a60fd-d1f9-48b5-9513-e4e535cf5d7c%2Flogs%2FTestProjects%2FVisualEffectGraph_HDRP%2Ftest-results/TestReportV1.html * Revert "*Fix HDRP warning" This reverts commit 99ab5fb. * More conservative way of workaround Preprocess issue Use a static cache of already santized object This solution remains a pretty dirty trick :-/ *Before* - Preprocess : Compile (possibly with side effect modifying the graph) - PostProcess : Sanitize, if there is a modification, it should relaunch the import & recompile *After* - Preprocess : Compile only if the sanitize has been done - PosProcess : Sanitize, if it's the first sanitize, relaunch the reimport Locally, it fixes unexpecting missing import of point cache asset (see 09_AttributeMaps). * Remove DestroyImmediate This instance singleton can be called from HDWizard constructor making this call illegal. See https://unity.slack.com/archives/GHD5LADU7/p1625040944440100?thread_ts=1624547150.387900&cid=GHD5LADU7 (cc @RSlysz) Testing yamato ⏳ * Revert "Remove DestroyImmediate " This reverts commit 508d713. * Add HDRPUserSettings It prevents Wizard Popup to be shown while runnning editor test See : https://unity.slack.com/archives/GHD5LADU7/p1625056568458400?thread_ts=1624547150.387900&cid=GHD5LADU7 * Revert "Add HDRPUserSettings" This reverts commit ff51dcf. * Force m_WizardPopupAtStart to false Prevent exception while launching test * Improvement of this workaround after discussion with @julienf - Use m_GraphSanitized instead of an HashSet - ClearRuntimeData when not sanitized (prevent any unexpected call to OnSetupMaterial) * Minor : Editor Test cleanup There was an unexpercted custom implementation of MakeTemporaryGraph Use common code : VFXTestCommon.MakeTemporaryGraph() * Gather all ImportAsset after all Sanitize from the same batch Fix this issue : #4971 (comment) See : See : https://unity.slack.com/archives/C47JPNNTZ/p1625218128286700?thread_ts=1623913383.239100&cid=C47JPNNTZ * Minor changes : Change log & Safe execption handling - Use a more explanotory message for the changelog - Handle exception safely to avoid the import cancellation - Fix issue : #4971 (comment) - Fix issue : #4971 (comment) * Fix sanitize issue during the very first loading GetOrCreateGraph is also assigning the visualEffectResource to the current graph https://github.com/Unity-Technologies/Graphics/blob/d6a59150d20762251cc5fcb8515fee494a142ccc/com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs#L231 It's needed to correctly update child dependencies This behavior is counterintuitive * *Minor* Replace `return` by `continue` I noticed this issue working on this PR : #5104 Co-authored-by: Kenny Tan <82013253+kennytann@users.noreply.github.com> Co-authored-by: Paul Demeulenaere <pauld@unity3d.com>

@RSlysz

….OnValidate (#5112) * [HDRP] Fix AxF debug output in certain configurations (#4641) * Fix AxF debug output in certain configurations. * Update comment * Fix SSR accumulation white flash (#4648) * Fix white flash * changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Display Info Box when MSAA + ray tracing is onr (#4627) * Show info box when ray tracing is enabled. * Changelog * Move below MSAA Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix distortion when resizing the window in player builds with the Graphics Compositor enabled (#4593) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Add support for the camera bridge in the graphics compositor (#4599) * Fix Jittered Project Matrix Infinite Far Clip Plane (#4638) * Reconstruct jittered projection matrix far plane (for Infinite ) * Changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fixed a memory leak related to not disposing of the RTAS at the end HDRP's lifecycle. (#4688) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix custom pass utils Blur + Copy overdraw. (#4623) * Fix overdraw in custom pass utils blur function * Updated changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix draw procedural invalid pass idx 1 on first template load (#4632) * Fix * changelog * Force sync compilation for TAA Co-authored-by: CifaCia <f.cifariellociardi@gmail.com> Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Changed light reset to preserve type (#4624) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Revert "Add support for the camera bridge in the graphics compositor (#4599)" This reverts commit 2325e3f. * AxF carpaint: Fix a compilation issue on Vulkan until the cpp HLSLcc side is updated. (case 1335737, related to 1314040) (#4691) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Revert "Revert "Add support for the camera bridge in the graphics compositor (#4599)"" This reverts commit 30fffd5. * revert: Fix distortion when resizing the window in player builds with the Graphi * Fixed the ray traced sub subsurface scattering debug mode not displaying only the RTSSS Data (case 1332904). (#4626) * Fixed the ray traced sub subsurface scattering debug mode not displaying only the RTSSS Data (case 1332904). * Add test scene Co-authored-by: Remi Chapelain <remi.chapelain@unity3d.com> Co-authored-by: Sebastien Lagarde <sebastien@unity3d.com> * Fix for discrepancies in saturation and intensity between screen space refraction and probe refraction (#4653) * Delete the second transmittance mul * Changelog Co-authored-by: Sebastien Lagarde <sebastien@unity3d.com> * Fix a Divide-by-Zero Warning for some Anisotropic Models (Fabric, Lit) (#4636) * Initialize the shading normal to a non-zero value for anisotropy * Changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * [HDRP] Fix VfX lit particle AOV output color space (#4646) * Fix VfX lit particle aov output color space * Update comment Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * [HDRP][Path Tracing] Fixed transparent unlit (#4605) * Fixed issue with transparent unlit. * Updated changelog. * Reverted accidental change to default mtl. Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix distortion with MSAA (#4711) * Fix contact shadow debug views (#4720) * Fix * changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Update Decal-Projector.md (#4695) * [HDRP] Fixed nullref when deleting the texture asset assigned in a local volumetric fog volume (#4728) * Fixed nullref when deleting the 3D mask of a density volume (case 1339330) * Updated changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix decal layer enum (#4753) * Fix typo * Fixed reflection probes being injected into the ray tracing light cluster even if not baked (case 1329083). (#4640) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Ignore hybrid duplicated reflection probes during light baking (#4663) * Ignore hybrid duplicated reflection probes during light baking * test path instead of scene Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix double sided option moving when toggling it in the material UI (#4725) * Fix double sided option moving when toggling it in the material UI (case 1328877) * Updated changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix formatting * Fix volumetric fog in planar reflections (#4736) * Fix planar reflection * changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix motion blur compute dispatch size (#4737) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * - Updated the recursive rendering documentation (case 1338639). (#4759) * - Updated the recursive rendering documentation (case 1338639). * review fixes Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix issue with OnDemand directional shadow map being corrupted when reflection probe are updated same frame (#4812) * Don't mark as rendered for reflection probes as we want the cached version to be from main view * Do the thing just for directional * Doc update * changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix cropping issue with the compositor camera bridge (#4802) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix for unused resources in depth of field (#4796) * Removing the word Radii from exposure settings (#4854) * Rename in UX * Update docs * [HDRP][Path Tracing] Support for shadow mattes (#4745) * Shadow matte support. * Updated changelog. * Only take occluders into account, closer match to raster mode. * Added test scene. Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Revert "[HDRP][Path Tracing] Support for shadow mattes (#4745)" This reverts commit 85ebbc2. * Fixed the transparent cutoff not working properly in semi-transparent and color shadows (case 1340234). (#4756) * Fixed the transparent cutoff not working properly in semi-transparent and color shadows (case 1340234). * Update test ref image Co-authored-by: Remi Chapelain <remi.chapelain@unity3d.com> Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Use non jittered projection in outline pass (#4836) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * [HDRP][Path Tracing] Sky settings now properly taken into account when using recorder (#4856) * Make sure sky settings are correctly set when recording. * Updated changelog. Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix Resolution Issues for Physically Based Depth of Field (#4848) * Add the necesarry texture coordinate clamping for RTHandle for color pyramid sampling * Add some resolution independence, fitted for 1920x1080 * Changelog * Switch back to point sampling from trilinear, with commentary * Update test reference images * Small correction to the point sampling, always sample mip 0. * Re-update the test images, for mip 0 color sampling * Use a simpler UV scaling/clamping since we are now point sampling. Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fixed bug introduced by sky-for-recorder support. (#4906) * [Fogbugz 1341576] Memory leaks when the player is paused, and the user changes pipeline… (#4845) * Memory leaks when the player is paused, and the user changes pipeline settings * changelog * [HDRP] Fixed shadergraph double save (#4916) * Don't need to save twice shadergraph the first time we create a graph * Updated changelog * Write 0 instead of micro sized motion vectors + fix extremely fast velocities (#4820) * Kill micromovements. * Do same for camera * Debug view update * Changelog * Remove unnecessary comment * Fix excessive velocity end up marked as no velocity Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * [HDRP] Fix material upgrader (#4821) * Fix HDRP material upgrade failing when there is a texture inside the builtin resources assigned in the material * Updated changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * [HDRP] Fix custom pass volume not executed in scene view (#4860) * Fix custom pass volume not executed in scene view because of the volume culling mask * Updated changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix reflection probe tootltip (#4890) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Updated the Physically Based Sky documentation for baked lights (#4891) * Updated the Physically Based Sky documentation for baked lights * Rewrite Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fixed remapping of depth pyramid debug (#4893) * Fixed remapping of depth pyramid debug * Removed debug pragma * Update changelog * Updated tooltip * Fix wobble-like (or tearing-like) SSAO issues when temporal reprojection is enabled. (#4895) * Fix AO perceived wobble * changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix Asymmetric Projection Matrices and Fog / Pathtracing (#4926) * Check for asymmetric projections and choose the generic path if so. * Fix asymmetric projections for the pathtracer ray generation. * Changelog * Simplify the matrix multiplication for computing the generic matrix. Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Revert: Fix wobble-like (or tearing-like) SSAO issues when temporal reprojection… * Fix GBuffer depth debug mode (#5054) * Fixed Volume Gizmo size when rescaling parent GameObject (#4915) * Fix Vertex Color Mode documentation (#4976) * Fix wobble-like (or tearing-like) SSAO issues when temporal reprojection is enabled. (#4986) * Fix AO perceived wobble * changelog * Update screenshots Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix formatting in NestedOverrideCameraRendering * [HDRP] Fix white flash with SSR when resetting camera history (#5089) * Fix white flash with SSR when resetting camera history * Move branch login inside GetPreviousExposureTexture * Fix for fixed exposure * [Fobguz # 1348357] VFX Particle templates missing stencil flags (#5080) * Adding missing macro for stencil flags in particle forward shaders. This will let flags like TAA Reject be useful * Changelog * Moved the range validation logic into HDAdditionalLightData.OnValidate (Case 1345264) * Update CHANGELOG.md * Merge master (#5127) * Fix case 1346650 where OnGUI draws were overwritten (#5107) * Fix case 1346650 where OnGUI draws were overwritten * update changelog * change conditional check * Revert "Color field keeps updating when using eye dropper and Esc key is pressed (#4950)" (#5122) This reverts commit 587dc15. * [VFX] Fix Sanitize behavior (#4971) * Add regression test on basic sanitize See https://unity.slack.com/archives/G1BTWN88Z/p1623825854027500 * Move SanitizeForImport See this conversation : https://unity.slack.com/archives/C47JPNNTZ/p1624277477261600?thread_ts=1623913383.239100&cid=C47JPNNTZ However, some concerns : - No idea about side effects of this solution - It will lead to a double compilation of sanitized files (if there is a sanitize needed, we will have a new reimport) - OnPostprocessAllAssets can't be filtered efficiently on the VisualEffectAsset * *Minor : Missing InvariantCultureIgnoreCase flag in EndsWith * *Update changelog.md * *Fix HDRP warning Hoping it will prevent this issue : ``` Unhandled log message: '[Exception] UnityException: DestroyImmediate is not allowed to be called from a ScriptableObject constructor (or instance field initializer), call it in OnEnable instead. Called from ScriptableObject 'HDWizard'. See "Script Serialization" page in the Unity Manual for further details.'. Use UnityEngine.TestTools.LogAssert.Expect ``` See https://yamato-artifactviewer.prd.cds.internal.unity3d.com/9f8a60fd-d1f9-48b5-9513-e4e535cf5d7c%2Flogs%2FTestProjects%2FVisualEffectGraph_HDRP%2Ftest-results/TestReportV1.html * Revert "*Fix HDRP warning" This reverts commit 99ab5fb. * More conservative way of workaround Preprocess issue Use a static cache of already santized object This solution remains a pretty dirty trick :-/ *Before* - Preprocess : Compile (possibly with side effect modifying the graph) - PostProcess : Sanitize, if there is a modification, it should relaunch the import & recompile *After* - Preprocess : Compile only if the sanitize has been done - PosProcess : Sanitize, if it's the first sanitize, relaunch the reimport Locally, it fixes unexpecting missing import of point cache asset (see 09_AttributeMaps). * Remove DestroyImmediate This instance singleton can be called from HDWizard constructor making this call illegal. See https://unity.slack.com/archives/GHD5LADU7/p1625040944440100?thread_ts=1624547150.387900&cid=GHD5LADU7 (cc @RSlysz) Testing yamato ⏳ * Revert "Remove DestroyImmediate " This reverts commit 508d713. * Add HDRPUserSettings It prevents Wizard Popup to be shown while runnning editor test See : https://unity.slack.com/archives/GHD5LADU7/p1625056568458400?thread_ts=1624547150.387900&cid=GHD5LADU7 * Revert "Add HDRPUserSettings" This reverts commit ff51dcf. * Force m_WizardPopupAtStart to false Prevent exception while launching test * Improvement of this workaround after discussion with @julienf - Use m_GraphSanitized instead of an HashSet - ClearRuntimeData when not sanitized (prevent any unexpected call to OnSetupMaterial) * Minor : Editor Test cleanup There was an unexpercted custom implementation of MakeTemporaryGraph Use common code : VFXTestCommon.MakeTemporaryGraph() * Gather all ImportAsset after all Sanitize from the same batch Fix this issue : #4971 (comment) See : See : https://unity.slack.com/archives/C47JPNNTZ/p1625218128286700?thread_ts=1623913383.239100&cid=C47JPNNTZ * Minor changes : Change log & Safe execption handling - Use a more explanotory message for the changelog - Handle exception safely to avoid the import cancellation - Fix issue : #4971 (comment) - Fix issue : #4971 (comment) * Fix sanitize issue during the very first loading GetOrCreateGraph is also assigning the visualEffectResource to the current graph https://github.com/Unity-Technologies/Graphics/blob/d6a59150d20762251cc5fcb8515fee494a142ccc/com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs#L231 It's needed to correctly update child dependencies This behavior is counterintuitive * *Minor* Replace `return` by `continue` I noticed this issue working on this PR : #5104 Co-authored-by: Kenny Tan <82013253+kennytann@users.noreply.github.com> Co-authored-by: Paul Demeulenaere <pauld@unity3d.com> Co-authored-by: Pavlos Mavridis <pavlos.mavridis@unity3d.com> Co-authored-by: FrancescoC-unity <43168857+FrancescoC-unity@users.noreply.github.com> Co-authored-by: sebastienlagarde <sebastien@unity3d.com> Co-authored-by: John Parsaie <johnpa@unity3d.com> Co-authored-by: anisunity <42026998+anisunity@users.noreply.github.com> Co-authored-by: Antoine Lelievre <antoinel@unity3d.com> Co-authored-by: CifaCia <f.cifariellociardi@gmail.com> Co-authored-by: Adrien de Tocqueville <adrien.tocqueville@unity3d.com> Co-authored-by: slunity <37302815+slunity@users.noreply.github.com> Co-authored-by: Remi Chapelain <remi.chapelain@unity3d.com> Co-authored-by: Emmanuel Turquin <emmanuel@turquin.org> Co-authored-by: Adrian1066 <44636759+Adrian1066@users.noreply.github.com> Co-authored-by: Kleber Garcia <kleber.garcia@unity3d.com> Co-authored-by: Julien Ignace <julien@unity3d.com> Co-authored-by: Kenny Tan <82013253+kennytann@users.noreply.github.com> Co-authored-by: Paul Demeulenaere <pauld@unity3d.com>

@RSlysz

* Add regression test on basic sanitize See https://unity.slack.com/archives/G1BTWN88Z/p1623825854027500 * Move SanitizeForImport See this conversation : https://unity.slack.com/archives/C47JPNNTZ/p1624277477261600?thread_ts=1623913383.239100&cid=C47JPNNTZ However, some concerns : - No idea about side effects of this solution - It will lead to a double compilation of sanitized files (if there is a sanitize needed, we will have a new reimport) - OnPostprocessAllAssets can't be filtered efficiently on the VisualEffectAsset * *Minor : Missing InvariantCultureIgnoreCase flag in EndsWith * *Update changelog.md * *Fix HDRP warning Hoping it will prevent this issue : ``` Unhandled log message: '[Exception] UnityException: DestroyImmediate is not allowed to be called from a ScriptableObject constructor (or instance field initializer), call it in OnEnable instead. Called from ScriptableObject 'HDWizard'. See "Script Serialization" page in the Unity Manual for further details.'. Use UnityEngine.TestTools.LogAssert.Expect ``` See https://yamato-artifactviewer.prd.cds.internal.unity3d.com/9f8a60fd-d1f9-48b5-9513-e4e535cf5d7c%2Flogs%2FTestProjects%2FVisualEffectGraph_HDRP%2Ftest-results/TestReportV1.html * Revert "*Fix HDRP warning" This reverts commit 99ab5fb. * More conservative way of workaround Preprocess issue Use a static cache of already santized object This solution remains a pretty dirty trick :-/ *Before* - Preprocess : Compile (possibly with side effect modifying the graph) - PostProcess : Sanitize, if there is a modification, it should relaunch the import & recompile *After* - Preprocess : Compile only if the sanitize has been done - PosProcess : Sanitize, if it's the first sanitize, relaunch the reimport Locally, it fixes unexpecting missing import of point cache asset (see 09_AttributeMaps). * Remove DestroyImmediate This instance singleton can be called from HDWizard constructor making this call illegal. See https://unity.slack.com/archives/GHD5LADU7/p1625040944440100?thread_ts=1624547150.387900&cid=GHD5LADU7 (cc @RSlysz) Testing yamato ⏳ * Revert "Remove DestroyImmediate " This reverts commit 508d713. * Add HDRPUserSettings It prevents Wizard Popup to be shown while runnning editor test See : https://unity.slack.com/archives/GHD5LADU7/p1625056568458400?thread_ts=1624547150.387900&cid=GHD5LADU7 * Revert "Add HDRPUserSettings" This reverts commit ff51dcf. * Force m_WizardPopupAtStart to false Prevent exception while launching test * Improvement of this workaround after discussion with @julienf - Use m_GraphSanitized instead of an HashSet - ClearRuntimeData when not sanitized (prevent any unexpected call to OnSetupMaterial) * Minor : Editor Test cleanup There was an unexpercted custom implementation of MakeTemporaryGraph Use common code : VFXTestCommon.MakeTemporaryGraph() * Gather all ImportAsset after all Sanitize from the same batch Fix this issue : #4971 (comment) See : See : https://unity.slack.com/archives/C47JPNNTZ/p1625218128286700?thread_ts=1623913383.239100&cid=C47JPNNTZ * Minor changes : Change log & Safe execption handling - Use a more explanotory message for the changelog - Handle exception safely to avoid the import cancellation - Fix issue : #4971 (comment) - Fix issue : #4971 (comment) * Add regression test (which is expecting to crash on master) * Fix sanitize issue during the very first loading GetOrCreateGraph is also assigning the visualEffectResource to the current graph https://github.com/Unity-Technologies/Graphics/blob/d6a59150d20762251cc5fcb8515fee494a142ccc/com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs#L231 It's needed to correctly update child dependencies This behavior is counterintuitive * *Revert this* : OnSRPChanged & PrepareSubgraphs are public * Not a valid fix : prepare subgraphs just before reimport * Rename ToSubGraphOperator to ConvertToSubGraphOperator * Experiment : trying with a force double import * Test : Don't know what PrepareSubgraph does better * Remove dirty PrepareSubgraphs in VFXViewWindow * *Add comment about reason behind PrepareSubgraphs requirement * *Update changelog Even if it's a bit ambitious for now... * Remove unexpected public/private * *Minor* Replace `return` by `continue` I noticed this issue working on this PR : #5104 * Extend test & Add some notes The missing TODOPAUL are revealed missing corner cases :loading: * Remove ClearRuntimeData Temp, it seems wrong but it's only related to the delayCall which let the VFXAsset in dangling state * *Add note about reason of failure for playmodeXR * Simpler solution : Use backup system instead of reimporting twice * Restore test which wasn't working before * Minor : update comment * *Fix bad conflict resolution * Restored previously skipped PrepareSubgraphs It has side effect which are caught by any editor test * Revert "Restored previously skipped PrepareSubgraphs" This reverts commit 2421bc5. * Fix missing ResyncSlots on the very first creation of the subgraph And verify expected output slot in VFXControllerTest Co-authored-by: julienf-unity <julienf@unity3D.com>

The regression has been probably introduced by #4971 What happens, 1st VFX import: - Not Sanitized - Skip first compilation - PostProcess => Sanitize & CheckGraphBeforeImport - Reimport Later, the ShaderGraph changes - Because the VFX is sanitized - Doesn't skip the first compilation => Fail - Post Process => CheckGraphBeforeImport is applied - ... Later VFX import will be ok until the SG isn't modified.

* Add Editor Test to cover issue 1361601 + Cleaner exception than a null ref exception * Actual fix from https://fogbugz.unity3d.com/f/cases/1361601/ The regression has been probably introduced by #4971 What happens, 1st VFX import: - Not Sanitized - Skip first compilation - PostProcess => Sanitize & CheckGraphBeforeImport - Reimport Later, the ShaderGraph changes - Because the VFX is sanitized - Doesn't skip the first compilation => Fail - Post Process => CheckGraphBeforeImport is applied - ... Later VFX import will be ok until the SG isn't modified. * *Update changelog.md * More readable exception when failing to find expression Equivalent change from e6d34d3 The following ContainsKey would have thrown an exception anyway * Move changelog entry to 13.x.x

* Add Editor Test to cover issue 1361601 + Cleaner exception than a null ref exception * Actual fix from https://fogbugz.unity3d.com/f/cases/1361601/ The regression has been probably introduced by #4971 What happens, 1st VFX import: - Not Sanitized - Skip first compilation - PostProcess => Sanitize & CheckGraphBeforeImport - Reimport Later, the ShaderGraph changes - Because the VFX is sanitized - Doesn't skip the first compilation => Fail - Post Process => CheckGraphBeforeImport is applied - ... Later VFX import will be ok until the SG isn't modified. * *Update changelog.md * More readable exception when failing to find expression Equivalent change from e6d34d3 The following ContainsKey would have thrown an exception anyway * Move changelog entry to 13.x.x # Conflicts: # com.unity.visualeffectgraph/CHANGELOG.md

PaulDemeulenaere added 4 commits June 24, 2021 14:24

Add regression test on basic sanitize

01e8e42

See https://unity.slack.com/archives/G1BTWN88Z/p1623825854027500

*Minor : Missing InvariantCultureIgnoreCase flag in EndsWith

e228f63

*Update changelog.md

4d22c94

github-actions bot added testing vfx labels Jun 24, 2021

PaulDemeulenaere added 9 commits June 24, 2021 16:39

Revert "*Fix HDRP warning"

d0d9438

This reverts commit 99ab5fb.

Remove DestroyImmediate

508d713

This instance singleton can be called from HDWizard constructor making this call illegal. See https://unity.slack.com/archives/GHD5LADU7/p1625040944440100?thread_ts=1624547150.387900&cid=GHD5LADU7 (cc @RSlysz) Testing yamato ⏳

Revert "Remove DestroyImmediate "

1fb0835

This reverts commit 508d713.

Add HDRPUserSettings

ff51dcf

It prevents Wizard Popup to be shown while runnning editor test See : https://unity.slack.com/archives/GHD5LADU7/p1625056568458400?thread_ts=1624547150.387900&cid=GHD5LADU7

Revert "Add HDRPUserSettings"

6c5f90b

This reverts commit ff51dcf.

Force m_WizardPopupAtStart to false

a569cbe

Prevent exception while launching test

Merge branch 'master' into vfx/fix/1344645-sanitize-failure

4f90950

PaulDemeulenaere commented Jul 1, 2021

View reviewed changes

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs Outdated Show resolved Hide resolved

PaulDemeulenaere commented Jul 1, 2021

View reviewed changes

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs Outdated Show resolved Hide resolved

Improvement of this workaround after discussion with @julienf

ded88dc

- Use m_GraphSanitized instead of an HashSet - ClearRuntimeData when not sanitized (prevent any unexpected call to OnSetupMaterial)

PaulDemeulenaere requested review from a team and julienf-unity July 1, 2021 13:26

VitaVFX requested review from VitaVFX and removed request for a team July 1, 2021 13:33

PaulDemeulenaere commented Jul 2, 2021

View reviewed changes

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs Outdated Show resolved Hide resolved

PaulDemeulenaere added 2 commits July 2, 2021 13:51

Minor : Editor Test cleanup

9e94d07

There was an unexpercted custom implementation of MakeTemporaryGraph Use common code : VFXTestCommon.MakeTemporaryGraph()

Gather all ImportAsset after all Sanitize from the same batch

8d4cdec

Fix this issue : #4971 (comment) See : See : https://unity.slack.com/archives/C47JPNNTZ/p1625218128286700?thread_ts=1623913383.239100&cid=C47JPNNTZ

PaulDemeulenaere commented Jul 2, 2021

View reviewed changes

TestProjects/VisualEffectGraph_HDRP/Assets/AllTests/Editor/Tests/VFXDataTests.cs Show resolved Hide resolved

PaulDemeulenaere marked this pull request as ready for review July 5, 2021 06:59

julienf-unity reviewed Jul 5, 2021

View reviewed changes

com.unity.visualeffectgraph/CHANGELOG.md Outdated Show resolved Hide resolved

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs Outdated Show resolved Hide resolved

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs Outdated Show resolved Hide resolved

Merge branch 'master' into vfx/fix/1344645-sanitize-failure

d17ddef

Unity-Technologies deleted a comment from github-actions bot Jul 7, 2021

julienf-unity approved these changes Jul 7, 2021

View reviewed changes

VladNeykov self-requested a review July 7, 2021 13:05

Merge branch 'master' into vfx/fix/1344645-sanitize-failure

5e6c3ea

PaulDemeulenaere mentioned this pull request Jul 8, 2021

[VFX] Crash with subgraph #5104

Merged

Unity-Technologies deleted a comment from github-actions bot Jul 9, 2021

Merge branch 'master' into vfx/fix/1344645-sanitize-failure

8f944a6

Unity-Technologies deleted a comment from github-actions bot Jul 9, 2021

VladNeykov approved these changes Jul 12, 2021

View reviewed changes

PaulDemeulenaere commented Jul 12, 2021

View reviewed changes

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs Outdated Show resolved Hide resolved

PaulDemeulenaere added 2 commits July 12, 2021 17:52

*Minor* Replace return by continue

8c442a8

I noticed this issue working on this PR : #5104

Merge branch 'master' into vfx/fix/1344645-sanitize-failure

2c0bde7

# Conflicts: # com.unity.visualeffectgraph/CHANGELOG.md

Unity-Technologies deleted a comment from github-actions bot Jul 12, 2021

Merge branch 'master' into vfx/fix/1344645-sanitize-failure

3aa94cc

VitaVFX approved these changes Jul 13, 2021

View reviewed changes

PaulDemeulenaere merged commit 42cfbe4 into master Jul 13, 2021

PaulDemeulenaere deleted the vfx/fix/1344645-sanitize-failure branch July 13, 2021 09:18

PaulDemeulenaere mentioned this pull request Aug 31, 2021

[VFX] Fix compilation error when shaderGraph is modified #5475

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[VFX] Fix Sanitize behavior #4971

[VFX] Fix Sanitize behavior #4971

Uh oh!

PaulDemeulenaere commented Jun 24, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PaulDemeulenaere commented Jul 5, 2021

Uh oh!

julienf-unity left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julienf-unity commented Jul 6, 2021

Uh oh!

PaulDemeulenaere commented Jul 9, 2021

Uh oh!

VladNeykov left a comment

Uh oh!

Uh oh!

github-actions bot commented Jul 13, 2021

Uh oh!

VitaVFX left a comment

Uh oh!

PaulDemeulenaere commented Jul 13, 2021

Uh oh!

Uh oh!

[VFX] Fix Sanitize behavior #4971

[VFX] Fix Sanitize behavior #4971

Uh oh!

Conversation

PaulDemeulenaere commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PaulDemeulenaere commented Jul 5, 2021

Uh oh!

julienf-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julienf-unity commented Jul 6, 2021

Uh oh!

PaulDemeulenaere commented Jul 9, 2021

Uh oh!

VladNeykov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jul 13, 2021

Uh oh!

VitaVFX left a comment

Choose a reason for hiding this comment

Uh oh!

PaulDemeulenaere commented Jul 13, 2021

Uh oh!

Uh oh!

PaulDemeulenaere commented Jun 24, 2021 •

edited

Loading