[VFX] reimport on SRP changed + remove built-in support and notify #4379

julienf-unity · 2021-04-29T12:49:42Z

Purpose of this PR

This PR uses the new SRP changed callback to reimport automatically VFX on SRP switch
It also removes built-in support (and all related code) and issue a warning if current pipeline is not supported by VFX

Limitation
There are a couple of cases where some VFX data can be lost when switching pipeline. Mostly slots value that are spawned based on some SRP caps (alphathreshold value for transparent outputs with motion vectors or shadows for instance)
Switiching SRP should not alter VFX asset in any way and this should be adress in another PR.

Side note
A callback should be added on SRP switch to inform user that some reimports will occur and this might take time

Testing status

Tested locally by switching pipeline asset

Comments to reviewers

This PR requires a C++ custom branch: https://ono.unity3d.com/unity/unity/pull-request/125169
At the moment this branch fails to compile with laltest master. I asked for a trunk merge
But if you encouter the compilation, just commenting out those in HDRenderPipeline.cs does the trick:
// , autoAmbientProbeBaking = false
// , autoDefaultReflectionProbeBaking = false
// , enlightenLightmapper = false

* Renable test to cover issue 1206890 * Fix newly introduced test : Exepcted value is original after reset override & handle correctly color

* Add int and uint to Compare node and condition expression * minor fixes * Change SerializeType so that tests are correct against System.Type and null * Fix switch * Update changelog

Co-authored-by: Lewis Jordan <lewis.jordan@hotmail.co.uk>

* Fix square complexity on parameter to serialized property matching * Update CHANGELOG.md

* Fix vfx view not beeing framed at launch * safer implementation * Update CHANGELOG.md

#84) * Fix for the lookup of mixed values in the VisualEffectEditor with objects that can be null * Update CHANGELOG.md

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

* Operator creation, does NOT sample an SDF * First implementation of the sampleSDF operator, might not be optimised * Delete VFXExpressionSampleSDF.cs * Updated Test Graph * Better handling of out of bounds queries for normals and distances * Delete HDRenderPipelineEditorResources.asset * Delete XRPackageSettings.asset * Assumption that the SDF is baked in the world space, in a box matching the orientedBox given as input * Update 27_SampleSDF.vfx * Update 27_SampleSDF.vfx * restored the deleted asset * Enforces and apply the 4 parents max rule * Removes the return statements in branch * Revert "restored the deleted asset" This reverts commit 57757b5a3ed4c303c34dc7dc920ad6d0328fcfdb. * Revert "Revert "restored the deleted asset"" This reverts commit 7753b4e85aba5d75e8af659aedc66515a0c56e20. * Fix compilation error and warnings * Fix Pascal case * Fix Pascal. Really. * Revert "Revert "Revert "restored the deleted asset""" This reverts commit daf8b79a78f55b47b6bd85afd9008a57d96f50f1. * Uses the Inverse-Transpose matrix for SDF related normal transformations (for non uniform scaling) * Delete packages-lock.json * Revert "Delete packages-lock.json" This reverts commit 4a10123f3171d6cf981c5effa52128ebb1411735. * Revert "Revert "Delete packages-lock.json"" This reverts commit 84d700f0f1fde91d4ad5a6ab87f3946216f883f4. * Restore this mistakenly modified file * Update 27_SampleSDF.vfx * Get rid of useless transpose * Added Graphic Test * Added reference images * Fix incorrect out-of-bounds check + auto recompile * Added reference images * Clarify the name of the output

* *Prepare a proper way to test different shadow cases * prepare a shadow graph unlit (modifying alpha) * Add reference data for shadow test * Minimal data for reproduce issue 1259511 * Fix shadowpath using common integration of PassDepthOrMV & adding VFX_PASSDEPTH_SHADOW define * *Update ShadowMaterial * *Update ShaderGraphShadow.vfx * *Move shader to common testing asset * Add equivalent test for URP * Add graphicTest for URP * *Update reference images * Fix display of alphaTreshold slider with shaderGraph : only if transparent using motionVector or shadowCasting * Fix alphaClipping condition (will require validation of @shader-graph) : use proper boolean synchronized instead of testing slot presence * Fix incorrect block listing * *Update changelog.md * Fix pass selection for lit effect * Early remove ports which aren't active in VFXTarget : save code compilation & cleaner solution * Voluntary add disabled branch on shadow unlit to cover alphaTreshold case * *Minor comment * Fix return alpha clipping (missing actual alpha test enabled) * *Restore deleted file (to minimize change in MR, these files can be directly deleted on vfx/staging) * VFX Graph Bugfix for shader compilation error in scene selection pass: 'Shaderpass should be defined at this stage.' # Conflicts: # com.unity.visualeffectgraph/Shaders/ParticlePlanarPrimitives/PassDepthOrMV.template * Revert unexpected change iVisualEffectGraph_HDRP\ProjectSettings\EditorBuildSettings.asset * *Update reference image & Fix treshold for URP scene which was really too low (set to default 5e-4f) * Fix issue https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/77#discussion_r69288 It was a bad merge Co-authored-by: pastasfuture <pastasfuture@gmail.com>

* Fix missing type copy in reduce Tested locally on a PS4 * Add expection to detect unsupported value type

* Change the way rand expression equality is handled * Refactor a bit * Add editor test

* World to Viewport Point & Viewport to World Point operators + Test First implementation of World to Viewport Point operator and its reciprocal Viewport to World Point operator. Includes a test scene for these 2 operators. * Update CHANGELOG.md Update Changelog * Added documentation md files. Co-authored-by: Lewis Jordan <lewis.jordan@hotmail.co.uk>

* Operator creation, does NOT sample an SDF * First implementation of the sampleSDF operator, might not be optimised * Delete VFXExpressionSampleSDF.cs * Updated Test Graph * Better handling of out of bounds queries for normals and distances * Delete HDRenderPipelineEditorResources.asset * Delete XRPackageSettings.asset * Assumption that the SDF is baked in the world space, in a box matching the orientedBox given as input * Update 27_SampleSDF.vfx * Update 27_SampleSDF.vfx * restored the deleted asset * Enforces and apply the 4 parents max rule * Removes the return statements in branch * Revert "restored the deleted asset" This reverts commit 57757b5a3ed4c303c34dc7dc920ad6d0328fcfdb. * Revert "Revert "restored the deleted asset"" This reverts commit 7753b4e85aba5d75e8af659aedc66515a0c56e20. * Fix compilation error and warnings * Fix Pascal case * Fix Pascal. Really. * Revert "Revert "Revert "restored the deleted asset""" This reverts commit daf8b79a78f55b47b6bd85afd9008a57d96f50f1. * Uses the Inverse-Transpose matrix for SDF related normal transformations (for non uniform scaling) * Delete packages-lock.json * Revert "Delete packages-lock.json" This reverts commit 4a10123f3171d6cf981c5effa52128ebb1411735. * Revert "Revert "Delete packages-lock.json"" This reverts commit 84d700f0f1fde91d4ad5a6ab87f3946216f883f4. * Restore this mistakenly modified file * Update 27_SampleSDF.vfx * Get rid of useless transpose * Added Graphic Test * Added reference images * Fix incorrect out-of-bounds check + auto recompile * Added reference images * Clarify the name of the output * Remove the use of inverse transpose for the "normals" * Restore the Inverse Transpose and modify Collision code * Restore Conform and Collide previous behavior + direction points to the surface * add documentation * Consistent distance and normal handling + ref images * Cleaning + World space stick distance and radius + references * Max scale through expression evaluated on GPU (if needed) * Max3 on CPU

…sions-colorgrading' into vfx/staging

* Simpler update to use now available ByteAddressBuffer * Temp Workaround a GPU hang We should isolate properly this code. * Proper fix of OOB FetchBuffer * *Update changelog.md

…fx-graphics into vfx/staging

* Fix VFXExpressionVector3sToMatrix & VFXExpressionVector4sToMatrix evaluation on GPU * *Update changelog.md

* Base Commit * Moved Files down one folder * Updated Package Configuration, CHANGELOG and documentation * Fixed Behavior of ExposedProperty by implementing a simple Drawer / Debug Behaviour * Small fixes and checks * Updated Documentation / Renamed Attribute handler RigidBody to RigidBodyVeolcity + Safe check * Small Fixes * Added the Execute In Editor Capability + Custom Inspectors in order to handle the capability correctly * Other custom Editors + Helpbox * Added CMCameraShake Editor + Capabilities / Fixed Possible Circular Reference in Prefab Spawn * Removed Rerouting Helpers as CopyValuesFrom is broken at the moment. * Fixed ASMDEFs / class accessibility * Updated Documentation * Fixed Class Accessibility for ExposedPropertyDrawer * Updated Changelog (missing entry for Exposed Proeprty Custom Property Drawer) Co-authored-by: Thomas ICHÉ <peeweek@gmail.com>

…hanged-callback

…changed-callback

…hanged-callback

github-actions · 2021-04-29T12:50:15Z

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page)
and cancel any jobs that started on Yamato.
See the PR template for more information.
Thank you!

loopervfx · 2021-05-02T05:19:39Z

This PR uses the new SRP changed callback to reimport automatically VFX on SRP switch
It also removes built-in support (and all related code) and issue a warning if current pipeline is not supported by VFX

@julienf-unity I don't see any problem with removing BRP (Built-in RP) support as long as #4030 is merged before this PR (or before a final release.) As I understand, it will maintain VFX Graph compatibility with BRP (in a new, render pipeline agnostic way,) now that #3467 has been merged?

I suggest to maintain continuous compatibility if at all possible, for to avoid a similar situation Unity had with Enlighten real-time GI support for HDRP:

x version 1.1 has legacy support for feature y
x version 1.2 removes legacy support for feature y
x version 1.3 adds new support for feature y

Ideally, the 2nd and 3rd step are swapped, avoiding final releases where the feature support is effectively missing. This could also be mitigated with backporting, but it seems it could be avoided entirely.

EDIT: Actually, I suppose it's already too late to avoid a lapse in BRP support. It looks like VFX graph support for BRP broke 3 months ago with #3443 (and even backported to previous releases) and was about to be fixed in #3950 but promptly canceled due to VFX Graph not "officially" supporting BRP.

Good thing most people don't know VFX Graph has (unofficially) supported BRP for years now 🤫

julienf-unity · 2021-05-05T16:13:45Z

@landonth SG supporting built-in does not imply VFX supporting builtin. Even with the new SG integration in VFX, we still have simulation shaders to generate that relies on SRP shader library for instance and also all our outputs that do not depend on new SG integration. So this PR means no builtin support in VFX. This has been discussed and it's approved to remove built in support in VFX (while shader graph is adding it... confusing I know). But maintaining built in in our case is too much of a burden.

So this PR officialy states "VFX not supported with built in" instead of being in an undefined territory with no clear communication (i.e we detect and compile for builtin but generated shaders are not compiling anymore since a few versions)

VladNeykov · 2021-05-05T18:46:20Z

Hi @julienf-unity , got some things jotted down on the test doc to take a look at when you get a chance.

VladNeykov

Looking good, one small (possibly unrelated) occasional console message case can be investigated down the road (noted on the test doc).
Otherwise tried switching between HDRP-URP-Built-in in various orders with many VFXs and it works as expected.
Attempting to use VFX in built-in provides an informative console message in the appropriate most common scenarios (SRP switch, project opening, creating new VFX, opening a VFX, and compiling a VFX).
No outstanding issues.

jenniferd-unity

Be careful with the fact an SRP can be active from the QualityLevel while GraphicsSettings are for a different SRP. (those workflow should not happen on future versions once the deprecation of GfxSettings' defaultRP lands)

jenniferd-unity · 2021-05-13T10:03:50Z

com.unity.visualeffectgraph/Editor/Core/VFXLibrary.cs

+        {
+            if (binder == null && (forceLog || !unsupportedSRPWarningIssued))
+            {
+                Debug.LogWarning("The Visual Effect Graph is supported in the High Definition Render Pipeline (HDRP) and the Universal Render Pipeline (URP). Please assign your chosen Render Pipeline Asset in the Graphics Settings to use it.");


it should also mention the Quality Settings (A valid Render Pipeline asset needs to be assigned in the Graphics Settings or the Quality Settings.)

So what would be the text? "Please assign your chosen Render Pipeline Asset in the Graphics or Quality Settings to use it" ?

This is what message the tech writing team gave to me for my feature "Project graphics settings do not refer to a URP Asset. Check the settings: Graphics > Scriptable Render Pipeline Settings, Quality > Render Pipeline Asset."
maybe it could be tweaked for your case to:
"The Visual Effect Graph is supported in the High Definition Render Pipeline (HDRP) and the Universal Render Pipeline (URP). Check the settings: Graphics > Scriptable Render Pipeline Settings, Quality > Render Pipeline Asset."

jenniferd-unity · 2021-05-13T10:06:50Z

com.unity.visualeffectgraph/Editor/Core/VFXLibrary.cs

+            bool logIssued = unsupportedSRPWarningIssued;
+            var binder = currentSRPBinder;
+
+            if (logIssued || !unsupportedSRPWarningIssued) // Don't reissue warning if inner currentSRPBinder call has already logged it


that if is a bit weird - it can probably be removed
logIssued == unsupportedSRPWarningIssued value according to line 498
and you are checking the value of unsupportedSRPWarningIssued in the other function

This is because we have 2 paths. LogUnsupportedSRP can be called from lazily initializing the currentSRPBinder (in that case we want to log only the first time) or explicitely upon some users action (opening the graph view, explicitely trying to compile a VFX...) and in this case we want to log always.

let us imagine unsupportedSRPWarningIssued equals to true. then
if(logIssue || !unsupportedSRPWarningIssued ) <=> if(true || false) > then TRUE
the opposite is:
if(logIssue || !unsupportedSRPWarningIssued ) <=> if (false|| true) > then TRUE

logIssued is just a cache to check whether the inner call to currentSRPBinder has issued the log already (logIssued is false and unsupportedSRPWarningIssued is true after the call) and in that case we don't reissue (else it will be logged twice in a row). true to false is not possible.

shouldnt it be forceLog in the if statement instead of logIssue?

If the test "if (logIssued || !unsupportedSRPWarningIssued)" is false, it means the message has already been log during the inner call to currentSRPBinder just above (unsupportedSRPWarningIssued was false before currentSRPBinder call, and true after), regardless of forceLog. This just prevents logging twice from the same explicit LogUnsupportedSRP call.

okayyyyyy.... currentSRPBinder is not a getter but a function call! now all of it makes sense :)
I was going crazy
so now... how do we prevent others from going crazy? :)

It's a getter but lazily initialized. We could move the initialization directly in the callback probably. But it was safer and less intrusive to do it this way now.

com.unity.visualeffectgraph/Editor/Core/VFXLibrary.cs

PaulDemeulenaere

Beside the remark from @jenniferd-unity
It looks good to me.
However, you should also double check your empty folder after master merge : the universal template folder has been moved to urp package, you could have an unexpected remaining metafile for the folder "RenderPipeline".

…hanged-callback

julienf-unity · 2021-05-17T20:03:23Z

We'll do a follow up PR to remove the reflection and polish things after C++ has landed

julienf-unity and others added 30 commits August 19, 2020 15:25

Merge remote-tracking branch 'graphics/master' into vfx/staging

84602be

Temporarily deactivate non uniform scale test (Some files were lost :()

f73d36d

[Test] Enable test to cover properly reset override behavior (#80)

a4c7bf3

* Renable test to cover issue 1206890 * Fix newly introduced test : Exepcted value is original after reset override & handle correctly color

int and uint support in Compare (#53)

3e771e2

* Add int and uint to Compare node and condition expression * minor fixes * Change SerializeType so that tests are correct against System.Type and null * Fix switch * Update changelog

Added render pipeline compatability table (#87)

453feba

Co-authored-by: Lewis Jordan <lewis.jordan@hotmail.co.uk>

Fix square complexity on parameter to serialized property matching (#78)

2981e68

* Fix square complexity on parameter to serialized property matching * Update CHANGELOG.md

Fix vfx view not beeing framed at launch (#79)

e600d59

* Fix vfx view not beeing framed at launch * safer implementation * Update CHANGELOG.md

Fix for the lookup of mixed values in the VisualEffectEditor with obj… (

42aa588

#84) * Fix for the lookup of mixed values in the VisualEffectEditor with objects that can be null * Update CHANGELOG.md

Merge branch 'master' into vfx/staging

ea2697f

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

Fix UInt/Int Condition Reduction (PS4/X1 issue) (#96)

292a833

* Fix missing type copy in reduce Tested locally on a PS4 * Add expection to detect unsupported value type

Fix 1154328 - infinite recompile with rand (#93)

dca3c30

* Change the way rand expression equality is handled * Refactor a bit * Add editor test

Make null ( not deserializable ) objects part of undo state (#86)

7ceeec9

Fix compile loop in vfxoperator (#61)

d2b86f6

Update to visual studio package 2.0.2 (#74)

a81c07b

Merge remote-tracking branch 'graphics/mobile/fast-srgb-linear-conver…

683a661

…sions-colorgrading' into vfx/staging

Fix Mesh Sampling for PS4/XOne (ByteAddressBuffer) (#91)

aba779d

* Simpler update to use now available ByteAddressBuffer * Temp Workaround a GPU hang We should isolate properly this code. * Proper fix of OOB FetchBuffer * *Update changelog.md

Merge branch 'vfx/staging' of github.cds.internal.unity3d.com:unity/v…

6f4d5a6

…fx-graphics into vfx/staging

Update assets

318b302

Update URP tests assets

6608c91

More update to assets

963d321

Remove old gfx tests images ref

64fcf79

Fix changelog

96ffc00

Fix Construct Matrix on GPU (#102)

293b0d4

* Fix VFXExpressionVector3sToMatrix & VFXExpressionVector4sToMatrix evaluation on GPU * *Update changelog.md

Add support for Gradient sampling with and without gradient mode #100

0958290

Fix changelog

f6fd8a5

julienf-unity added 5 commits April 28, 2021 16:57

Merge remote-tracking branch 'graphics/master' into vfx/feature/srp-c…

62a9c1a

…hanged-callback

Implement inner callback mechanism

c37b304

Merge branch 'vfx/feature/srp-support-feedback' into vfx/feature/srp-…

a4bb699

…changed-callback

Misc small fixes + remove logs

3300826

Merge remote-tracking branch 'graphics/master' into vfx/feature/srp-c…

d83db5f

…hanged-callback

github-actions bot added the vfx label Apr 29, 2021

julienf-unity requested review from a team, PaulDemeulenaere and jenniferd-unity April 29, 2021 12:53

Fix issues from new SG integration

a315e56

PaulDemeulenaere mentioned this pull request May 6, 2021

[VFX/URP] Lit Output #4353

Merged

Added unsupported SRP warning log in some places

3882ced

VladNeykov requested review from VladNeykov and removed request for a team May 12, 2021 08:55

VladNeykov approved these changes May 12, 2021

View reviewed changes

jenniferd-unity suggested changes May 13, 2021

View reviewed changes

PaulDemeulenaere approved these changes May 17, 2021

View reviewed changes

julienf-unity added 2 commits May 17, 2021 11:37

Merge remote-tracking branch 'graphics/master' into vfx/feature/srp-c…

a824274

…hanged-callback

Check QualitySettings for current SRP

1e68bf8

jenniferd-unity approved these changes May 17, 2021

View reviewed changes

julienf-unity added 2 commits May 17, 2021 17:01

Merge branch 'master' into vfx/feature/srp-changed-callback

dcbffc1

Fix formatting

f95c130

julienf-unity merged commit d6a5915 into master May 17, 2021

julienf-unity deleted the vfx/feature/srp-changed-callback branch May 17, 2021 20:03

jenniferd-unity mentioned this pull request May 19, 2021

Changed RenderPipelineManager.activeRenderPipelineTypeChanged from internal to public #4572

Merged

[VFX] reimport on SRP changed + remove built-in support and notify #4379

[VFX] reimport on SRP changed + remove built-in support and notify #4379

Uh oh!

Conversation

julienf-unity commented Apr 29, 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!

github-actions bot commented Apr 29, 2021

Uh oh!

loopervfx commented May 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julienf-unity commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VladNeykov commented May 5, 2021

Uh oh!

VladNeykov left a comment

Choose a reason for hiding this comment

Uh oh!

jenniferd-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PaulDemeulenaere left a comment

Choose a reason for hiding this comment

Uh oh!

julienf-unity commented May 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

julienf-unity commented Apr 29, 2021 •

edited

Loading

loopervfx commented May 2, 2021 •

edited

Loading

julienf-unity commented May 5, 2021 •

edited

Loading