[VFX] Direct Link Event & Multiple Per Frame #4457

PaulDemeulenaere · 2021-05-05T19:00:14Z

Purpose of this PR

Adding support of direct link event which also support multiple event within the same frame.

See this presentation : It only covers "Direct Link" until slide 16 ⬇️

N.B.: In this view ⬆️, spawnCount should be a float (instead of UInt), it's always expressed as a float in eventAttributes.

⚠️ Require this C++ change : https://ono.unity3d.com/unity/unity/pull-request/125497/_/graphics/vfx/fix/multiple-event-per-frame

Testing status

Added two tests

DirectLinkMix : Simulates several constant rate through simple code
DirectLinkRLE : SendEvent multiple event per frame drawing the content (there are 408 "chunk" in this image it leads to 20 SendEvent per frame)

@unity/gfx-qa-vfx This change can't really be tested without a test script, you can be inspired by these two ⬆️
However, there is one way of testing without script : the default spawnCount is now "1.0f"

Comments to reviewers

First QA pass has been already be done in internal repository : See this report from @VitaSkruibyte

About side fixes (I will prepare an independent backport PR if needed)

⚠️ This PR also include this one : https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/160 (I changed the base to reduce file change) because I need to have a way to override the default spawnCount from 0.0f to 1.0f. See this change
Sum up here ⬇️

Purpose of this PR :

Fix unexpected spawn context order of execution when graph isn't balanced
Add explicit access of spawnCount, it prevents workaround or custom spawner (see also this document)

Relative to these changes :

Remove GetSpawnCount.cs and replace this by an explicit "Get Source Spawn Count".
Remove some hacks (and also dead code, see FillEventAttributeDescs), since spawnCount can be directly used.
Manual test for sanitize of GetSpawnCount (which can now explicitly be accessed)
Add editor test to check the behavior of previously incorrect CollectContextParentRecursively (at dda2404)
Debug manually content for Graphic Test using chaining
20_SpawerChaining has been impacted : Both ordering are actually correct but since we are using random in spawnContext, it change a bit the rendering ⬇️
Before

After
009_ReadAttribute is impacted, there was an untracked mistake on this one : the "debug" information was one frame behind (here represented by a green square, it's a kind of cursor) :

Fix issue https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/131#discussion_r79026

@LewisJordan

* Fix missing doc & Deprecate VFXViewModicationProcessor Unexpected public API * *Revert unexpected change * Fix several issue from @LewisJordan - fix issue https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/143#discussion_r79397 - fix issue https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/143#discussion_r79398

* Fix case 1274790 : Use DestroyImmediate when editor isn't playing * Fix case 1247230 : Avoid null exposed property Property entry is modified by UX always not null, however, it requires a default value * Fix case 1274788 : Missing null check in Hierarchy to Attribute Map * Fix case 1248711 : Property window view wasn't working Missing Update, it also fixes Undo/Redo * *Update changelog * Fix compilation when ENABLE_LEGACY_INPUT_MANAGER isn't enabled Should be fix for 10.2 * Fix case 1279471 : Avoid OnMouseDown/Up/... declaration Game scripts or other custom code contains OnMouse_ event handlers. Presence of such handlers might impact performance on handheld devices." when building for Android/IOS

…fx-graphics into vfx/staging

…ide mode. (#145)

…fx-graphics into vfx/staging

* Base refactor + Attempt to get position from AABox (not working for thickness ATM) * Fixed cone syntax issues + correct computation of AABox direction * Added Variant providers + Composition in Sequential * Updated Variants for Shape Sequential Blocks * Harmonized Namings + added composition to Position Depth * Updated Changelog * Fixes for PR * Fixed Blend Composition in Sequential * Added Direction to PositionSequential * Fixes in Position Circle / Set Blend factor in shapes to 1.0 by default * Used Absolute Box size as expression * Propal for https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/44/files/e029bb9ec37555e70af21a97399774667030c031#r61533 Use case by case approach for direction of AABox * *Temp add test data for graphicTest * Fix ApplyAddressingMode : clamp & mirror was overflowing, mirror has also a wrong pattern * Edit graphicTest * Move 014 to common package * Add 014_PositionBlock in editor test listing * Precompute line_direction in PositionLine Fix issue https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/44/files#r67908 * *Add reference images * Fix editor test (wrong reference data) * Fix issue introduced at 9c54056 : Looping correctly the circle See also : https://unity.slack.com/archives/G1BTWN88Z/p1598508170069200?thread_ts=1598429838.039200&cid=G1BTWN88Z * Probably uploaded the wrong image reference for standalone * *Update reference images (I think I mess up twice, I should double check the change in motionVector) * Fix build (VFXExpressionCondition now supports uint) * *Temp* Delete motion vector reference image, should regenerate them from yamato. * Readd reference image using yamato result at b9a04b7 * *Update changelog Fix issue : https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/44#discussion_r70582 * Fix PositionTorus when used in vertex shader * Fix multiple definition of UNITY_PI * Fix changelog bad automatic merge * Fix incorrect volume Base radius while computing volume factor on sphere & circle : use fix approach from https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/89 using name instead of index See : https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/44#issuecomment-28990 & https://docs.google.com/document/d/1RAVkfmMQA9D_hKkJyt6PKOgRs8JHkjlYRhcfBtIU1Ag/edit?disco=AAAAJ7Z4S18 Co-authored-by: Thomas ICHÉ <peeweek@gmail.com> Co-authored-by: Paul Demeulenaere <pauld@unity3d.com> Co-authored-by: Julien Fryer <julienf@unity3d.com>

…eturn proper values (#141) * Update VFXShaderGraphParticleOutput.cs * Update CHANGELOG.md Co-authored-by: Julien Fryer <julienf@unity3d.com>

* Diffusion profile has PropertyType.Float but isn't castable in Vector1ShaderProperty : safeprof code * Unity serialization produced a beast with object.ReferenceEquals(null) != 0 and instanceID == 0 foolProof code for this * Update CHANGELOG.md

* Prevent exceptions from making notification stay. Prevent exception in errors to potential break the ui * Prevent list from being badly deserialized.

* Add missing filter in CanLink function * Add editor test * *Update changelog.md * *Update comment * *Update Comment

* Vfx/feature/strips improvements (#265) * Add spanwIndexInStrip attribute * Fix particleCountInStrip in Init * Add OrientStrip block * Remove some useless code * Add From Target Position mode in Orient Strip * Update changelog * Fix SpawnOverDistance * Fix reference images * Fix changelog * Merge Orient and OrientStrip blocks * Remove FaceCameraPlane from strips * Sanitize quad strip orientation * Update VFX * Update changelog * Add error feedback * Use IsPerspectiveProjection * Fix GetStripTangent for lines * Add subpixel AA to head and trails template system * Add strip variant for intialize in menu * Fix for errors not appearing after convert output * Update graph version * Fix ribbon VFX * Fix spawnIndexInStrip (tooltip + init) * Fix issue with inspector not triggering OnSettingChanged * Check that a converted output could keep the same flow links. Co-authored-by: Tristan Genevet <tristan@unity3d.com>

* Updated Changelog * Updated Blocks with Fixes and Missing Workflows * update VFX additions Co-authored-by: Thomas Iché <peeweek@gmail.com>

…put Particle Mesh / Connected Attributes (#139) * Reindex the TEXCOORD[n] * Update CHANGELOG.md Co-authored-by: Julien Fryer <julienf@unity3d.com>

…fx-graphics into vfx/staging

…wncount

…fix/multiple-event-per-frame

- Need to reference SystemHasDirectLink (could be determined another statically in C++ but it's simpler and faster this way) - Remove need of readsource - Remove GenerateComputeSourceIndex - Force add spawnCount in globalAttribute : it isn't an implicit need anymore

See https://docs.google.com/document/d/10AbwF_bsv0O45NhFMxq4Y57EGahhf0aSBT0BxKEIHpE/edit?disco=AAAALojHD4Y

# Conflicts: # TestProjects/VisualEffectGraph_HDRP/ProjectSettings/EditorBuildSettings.asset # TestProjects/VisualEffectGraph_URP/ProjectSettings/EditorBuildSettings.asset # com.unity.visualeffectgraph/CHANGELOG.md # com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs

julienf-unity

I think the indexInSystem could be removed and simplify a lot. Here the way it is done makes supposely immutable data to be written to + it's error prone as some typos are apparently here

com.unity.visualeffectgraph/Editor/Data/VFXData.cs

com.unity.visualeffectgraph/Editor/Compiler/VFXGraphCompiledData.cs

com.unity.visualeffectgraph/Editor/Data/VFXDataOutputEvent.cs

com.unity.visualeffectgraph/Editor/Compiler/VFXGraphCompiledData.cs

com.unity.visualeffectgraph/Editor/Data/VFXDataMesh.cs

com.unity.visualeffectgraph/Editor/Data/VFXDataParticle.cs

... but actually, we can't identify spawn context by VFXData (see the following commit)

Fix spawner chaining issue from 3709c18

This method is really specific to VFXDataParticle, it doesn't require any abstraction.

PaulDemeulenaere · 2021-05-12T13:21:49Z

About the removal of indexInSystemDesc, it has been done by these two changes :

3709c18 : Using VFXData as unique identifier
5854147 : Using VFXContext as unique identifier (VFXData is shared amond chained spawn context)

Tested editor & playmode test.

VitaVFX

Looks good, found one insignificant issue but this can be looked into after the feature cut off. Here's the test doc

Missing change from revision : aa85881

# Conflicts: # com.unity.visualeffectgraph/CHANGELOG.md # com.unity.visualeffectgraph/Editor/Data/VFXDataParticle.cs

PaulDemeulenaere · 2021-05-17T11:28:35Z

All nightly VFX verification are 🟢 at this revision : c71c695
Awaiting for the formatting job before merging ⏳

Mistake introduced at #4457

* Fix incorrect condition for GPUEvent Mistake introduced at #4457 * *Add some editorTest * Add some test to covers corners case See https://unity.slack.com/archives/G1BTWN88Z/p1632919330247400 * *WIP* Improve incorrect connection handling * Fix multiple type in output of initialize part * Fix Update to Update which is actually allowed * *Update changelog.md * Fix missing case for VFX.Update tyep * Rename confusing "primaryType"

julienf-unity and others added 30 commits October 20, 2020 17:16

More changelog fixes

4ad6493

Add missing warning for other block operator using depth (#140)

08ff952

Fix issue https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/131#discussion_r79026

*Adding missing meta file in TestProject

75b4f58

Merge branch 'master' into vfx/staging

88af45c

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

660b777

…fx-graphics into vfx/staging

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

3e72ca9

Fix positionDepthOperator ref images

51f3d7d

Add one pixel to height in non wide mode so that margin is equal to w…

15e6ed8

…ide mode. (#145)

Update CHANGELOG.md

644bdde

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

8b0397e

Fix API validation

d1be1a1

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

bba06b6

…fx-graphics into vfx/staging

Fix 1285603 View and Object mode of Shadergraph position node don't r…

c81ea3e

…eturn proper values (#141) * Update VFXShaderGraphParticleOutput.cs * Update CHANGELOG.md Co-authored-by: Julien Fryer <julienf@unity3d.com>

Fix collapse exception and added automated test (#149)

ea805dc

Small fix for collapse test

b8212d0

Some extra security around error feedback (#153)

182437a

* Prevent exceptions from making notification stay. Prevent exception in errors to potential break the ui * Prevent list from being badly deserialized.

Fix Event connected directly to Output Event (#152)

adaeba7

* Add missing filter in CanLink function * Add editor test * *Update changelog.md * *Update comment * *Update Comment

Revert adaeba7

024dcbf

update vfx

3801a89

Update URP assets

20a4672

Update URP ref

61b8375

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

f6ee116

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

818adb6

Vfx/fix/additional subgraphs 2 (#155)

ef6eeaf

* Updated Changelog * Updated Blocks with Fixes and Missing Workflows * update VFX additions Co-authored-by: Thomas Iché <peeweek@gmail.com>

Fix 1255182 Compile errors using Baked GI (default UV settings) / Out…

15df881

…put Particle Mesh / Connected Attributes (#139) * Reindex the TEXCOORD[n] * Update CHANGELOG.md Co-authored-by: Julien Fryer <julienf@unity3d.com>

julienf-unity and others added 9 commits March 25, 2021 16:13

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

70b8151

Merge branch 'vfx/staging' into vfx/fix/multiple-event-per-frame

a90b462

Changed mobile to Android (#227)

dde0fcc

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

9d51001

…fx-graphics into vfx/staging

Merge branch 'vfx/staging' into vfx/feature/allow-modify-directly-spa…

1c87122

…wncount

Merge branch 'vfx/feature/allow-modify-directly-spawncount' into vfx/…

250b76d

…fix/multiple-event-per-frame

Fix incorrect IsExclusiveLink condition

c6f15ad

See https://docs.google.com/document/d/10AbwF_bsv0O45NhFMxq4Y57EGahhf0aSBT0BxKEIHpE/edit?disco=AAAALojHD4Y

github-actions bot added testing vfx labels May 5, 2021

PaulDemeulenaere requested review from VitaVFX and julienf-unity May 5, 2021 19:04

julienf-unity reviewed May 11, 2021

View reviewed changes

PaulDemeulenaere added 3 commits May 12, 2021 14:39

Remove hacky indexInSystemDesc

3709c18

... but actually, we can't identify spawn context by VFXData (see the following commit)

Use VFXContext identifier instead of VFXData

5854147

Fix spawner chaining issue from 3709c18

Remove hasDynamicSourceCount

aa85881

This method is really specific to VFXDataParticle, it doesn't require any abstraction.

VitaVFX approved these changes May 12, 2021

View reviewed changes

PaulDemeulenaere added 4 commits May 12, 2021 17:14

Fix build

aa49f69

Missing change from revision : aa85881

*Update missing reference images (& modified)

9f8a25b

Merge branch 'master' into vfx/fix/multiple-event-per-frame

467b5fe

# Conflicts: # com.unity.visualeffectgraph/CHANGELOG.md # com.unity.visualeffectgraph/Editor/Data/VFXDataParticle.cs

Merge branch 'master' into vfx/fix/multiple-event-per-frame

c71c695

julienf-unity approved these changes May 17, 2021

View reviewed changes

PaulDemeulenaere marked this pull request as ready for review May 17, 2021 11:28

PaulDemeulenaere merged commit 405f572 into master May 17, 2021

PaulDemeulenaere deleted the vfx/fix/multiple-event-per-frame branch May 17, 2021 11:57

PaulDemeulenaere added a commit that referenced this pull request Sep 29, 2021

Fix incorrect condition for GPUEvent

18896a6

Mistake introduced at #4457

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VFX] Direct Link Event & Multiple Per Frame #4457

[VFX] Direct Link Event & Multiple Per Frame #4457

PaulDemeulenaere commented May 5, 2021 •

edited

Loading

julienf-unity left a comment

PaulDemeulenaere commented May 12, 2021

VitaVFX left a comment

PaulDemeulenaere commented May 17, 2021 •

edited

Loading

[VFX] Direct Link Event & Multiple Per Frame #4457

[VFX] Direct Link Event & Multiple Per Frame #4457

Conversation

PaulDemeulenaere commented May 5, 2021 • edited Loading

Purpose of this PR

Testing status

Comments to reviewers

About side fixes (I will prepare an independent backport PR if needed)

julienf-unity left a comment

Choose a reason for hiding this comment

PaulDemeulenaere commented May 12, 2021

VitaVFX left a comment

Choose a reason for hiding this comment

PaulDemeulenaere commented May 17, 2021 • edited Loading

PaulDemeulenaere commented May 5, 2021 •

edited

Loading

PaulDemeulenaere commented May 17, 2021 •

edited

Loading