[VFX] Additional Sorting Keys [22.1] #5125

ludovic-theobald · 2021-07-13T09:20:39Z

[VFX] Additional Sorting Keys

Checklist for PR maker

Have you added a backport label (if needed)? For example, the need-backport-* label. After you backport the PR, the label changes to backported-*.
Have you updated the changelog? Each package has a CHANGELOG.md file.
Have you updated or added the documentation for your PR? When you add a new feature, change a property name, or change the behavior of a feature, it's best practice to include related documentation changes in the same PR. If you do add documentation, make sure to add the relevant Graphics Docs team member as a reviewer of the PR. If you are not sure which person to add, see the Docs team contacts sheet.
Have you added a graphic test for your PR (if needed)? When you add a new feature, or discover a bug that tests don't cover, please add a graphic test.

Purpose of this PR

This PR introduces new criteria to sort the particles of an output context. Prior to this PR, if sorting was enabled, particles were sorted by their distance to the camera. We now propose a total of 5 sorting criteria :

By Distance (previously, the only option)
Youngest in front
Oldest in front
By Depth
Custom
- the Custom criterion creates a new slot in the Output context, where the user can plug a sorting key which will be used to sort the particles.

The sorting criterion can be chosen in the inspector if the Sort is set to On or Auto.

Notes:

We cannot sort particles from different outputs, meaning that if you have two outputs to a system, one will appear completely in front or completely behind, depending on their sort priority.
When possible (outputs sharing the same sort criterion), only one global sort is issued for all the outputs. If outputs have different sorting criteria, they will require their own sort pass, incurring an additional cost in performance.
This bug causing flickering when there are more than 4096 particles is fixed in this PR.

JIRA story

This PR required modification of the C++ code.

Link to C++ PR : https://ono.unity3d.com/unity/unity/pull-request/130921/_/graphics/vfx/feature/new-sortkeys

Testing status

What has been tested :

Sorting 1 output (simple quads) with all the sorting criterion
Sorting up to 4 outputs, with heterogeneous sorting criteria
Sorting two multimesh outputs
Only in HDRP (but shouldn't be dependant on the SRP)

What hasn't been tested :

Different platforms
Extensive test on the custom sort key slot (e.g. will it break with some inputs?)
Upgrading from an older version

I plan to add a graphics test for this feature too.

Comments to reviewers

github-actions · 2021-07-13T09:20:57Z

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)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

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.

VladNeykov · 2021-07-27T18:15:53Z

Hi, dropping the WIP Test Doc with a few items already that can be looked at.

julienf-unity

In progress

julienf-unity · 2021-08-16T15:54:47Z

com.unity.visualeffectgraph/Shaders/Sort.compute

    // Select left or right: Must handle special case for equality
-    bool select0 = (key0 == key1 && (id & 1)) || (CompareKeys(key0,key1) != reverse);
-    uint value = inputSequence[(select0 ? index0 : index1)].value;
+    bool select0 = (kvp0.key == kvp1.key && kvp0.value == kvp1.value && (id & 1)) || (CompareKVP(kvp0, kvp1) != reverse);


The first condition does not seem relevant anymore. CompareKVP test seems enough

com.unity.visualeffectgraph/Shaders/Sort.compute

…ure/new-sorting-keys

ludovic-theobald · 2021-10-04T08:45:02Z

any update on this PR?

C++ side of the PR is currently being reviewed

I've made some changes to the new sections. Please double-check for accuracy.

Vic-Cooper

I approve my own changes :D

julienf-unity · 2021-10-27T15:17:46Z

com.unity.visualeffectgraph/Shaders/VFXOutputUpdate.template

                    return;


 #if VFX_FEATURE_SORT


Couldnt all this be factorized in some way with what's in GlobalSortKey.template?

julienf-unity · 2021-10-27T15:30:46Z

com.unity.visualeffectgraph/Editor/Models/Contexts/VFXOutputUpdate.cs

+                        var sortKeyExp = m_Output.inputSlots.First(s => s.name == "sortKey").GetExpression();
+                        expressionMapper.AddExpression(sortKeyExp, "sortKey", -1);
+                    }
+                    catch (Exception e)


Isnt this try catch just a temp debug things?

julienf-unity · 2021-10-27T15:36:52Z

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

            var implicitContext = new List<VFXContext>();
-            if (NeedsGlobalSort())
+            bool needsGlobalSort = NeedsGlobalSort();
+            //Issues a global sort, when it affects at least one output.


Shouldnt the global sort be used when it affects at least 2 outputs, for 1 output using UpdateOutput pass seems better if any as it potentially removes 1 dispatch (+ takes into account attribute modification in output if any)

# Conflicts: # com.unity.visualeffectgraph/Shaders/VFXCameraSort.template

PaulDemeulenaere

Approving but only because I'm following the landing of this PR (see #6172)

...rojects/VisualEffectGraph_HDRP/Assets/AllTests/VFXTests/GraphicsTests/16_MeshParticles.unity

Needed after merge of #5125

* renaming/shader code for built-in sort keys * New task types + sorting logic + sort with value * Vote to choose the global sorting (wip) * Also compare the custom slot if needs be * Fix binding buffer issues * Fix needsOwnSort Condition * use needsOwnSort for MeshOutputs * Reformatting * Sort order consistent with other sortings * Warnings for changes in output * New graphics test * Always add isPerCameraSort mapping * sort mode instead of criterion + tooltips * small renaming * remove useless condition + cleaner ternary op * restore ternary operator (error on user defined types) * local reformatting * fix bad merge * Fix naming in filteredSettings + strips case * change feedback text to workaround layout issue * Wrongly public method * Error feedback only when block is enabled * Fix Output ParticleStrip having sort available when created * Update VFX to correct criterion * Criteria renaming (explicit "ToCamera") * Cleanup voting mechanism * more cleanup * fix missing comparer * expose the option (does nothing) * little cleaning * moved helper function * EqualityComparer instead of the interface * Unify sorting criteria * Simplify logic * keep SortKeySlotComparer internal * Reuse SortingCriteriaComparer for needOwnSort * Activate inversion in shader * Activate inversion in shader (bis) * reformat file * Remove OldestInFront * Forces Per Output update when there is no Update but Sorting is on * Shut down intermediate return (buggy) warning * Warning when age is not set * Add reference images * Add reference image + update affected tests * Attributes -> VFXAttributes * Correct cast * Update docs * update changelog * Docs review. I've made some changes to the new sections. Please double-check for accuracy. * Fixed *Sort* definition * Same amount of mappings for global/peroutput * Fix merge * Remove try/catch * Two similar criterion for global sort * Return global sorting criterion only when needed * FeedSortingKeys factorized * Fix needsOwnSort condition * add the graphics test * Fix test failure * Fix metal refs * exclude 16_MeshParticles in XR Co-authored-by: Vic-Cooper <vic.cooper@unity3d.com>

This PR backports some changes from the [Additional sorting keys PR](#5125) that fixes the flickering that happens for systems with capacity >4096. It also includes some optimizations introduced in this [fix](https://ono.unity3d.com/unity/unity/pull-request/139471/_/graphics/vfx/1300251-bitonic-sort-low-end). Please note that it is **not** a full backport of the Additional Sorting keys feature.

ludovic-theobald added 7 commits July 2, 2021 16:21

renaming/shader code for built-in sort keys

0afa885

New task types + sorting logic + sort with value

a464578

Vote to choose the global sorting (wip)

7b5eee9

Also compare the custom slot if needs be

28fedf5

Fix binding buffer issues

4356eb5

Fix needsOwnSort Condition

ea1d7a2

use needsOwnSort for MeshOutputs

cc123bb

github-actions bot added HDRP vfx labels Jul 13, 2021

ludovic-theobald requested review from a team and julienf-unity July 13, 2021 09:21

VladNeykov requested review from VladNeykov and removed request for a team July 13, 2021 09:31

ludovic-theobald requested a review from peeweek July 13, 2021 11:41

Reformatting

9db719e

ludovic-theobald marked this pull request as ready for review July 26, 2021 07:25

ludovic-theobald added 6 commits July 28, 2021 13:23

Sort order consistent with other sortings

8f62241

Warnings for changes in output

bdab9bd

New graphics test

77d3437

Always add isPerCameraSort mapping

c4ad9e9

sort mode instead of criterion + tooltips

15f795d

small renaming

a5becb5

julienf-unity reviewed Aug 16, 2021

View reviewed changes

ludovic-theobald added 4 commits August 18, 2021 10:33

remove useless condition + cleaner ternary op

213df61

restore ternary operator (error on user defined types)

5e03988

Merge commit 'a8abe4c5d857cbada0535ec3a5ca770265b639ed' into vfx/feat…

43fd648

…ure/new-sorting-keys

local reformatting

d3e8156

ludovic-theobald requested a review from a team as a code owner August 18, 2021 09:04

update changelog

6cc5dd2

ludovic-theobald requested a review from Vic-Cooper October 4, 2021 08:44

Vic-Cooper added 2 commits October 5, 2021 11:35

Docs review.

3ee28d3

I've made some changes to the new sections. Please double-check for accuracy.

Fixed *Sort* definition

e061bbf

Vic-Cooper approved these changes Oct 5, 2021

View reviewed changes

ludovic-theobald added 2 commits October 11, 2021 14:16

Same amount of mappings for global/peroutput

2c994ba

Merge branch 'master' into vfx/feature/new-sorting-keys

34d3838

julienf-unity reviewed Oct 27, 2021

View reviewed changes

ludovic-theobald added 9 commits October 29, 2021 09:32

Merge branch 'master' into vfx/feature/new-sorting-keys

707bf65

# Conflicts: # com.unity.visualeffectgraph/Shaders/VFXCameraSort.template

Fix merge

2b4c05d

Remove try/catch

ec26cf9

Two similar criterion for global sort

37b6f8e

Return global sorting criterion only when needed

48006b8

FeedSortingKeys factorized

8f54091

Fix needsOwnSort condition

a11b0d8

add the graphics test

7ea4b9f

Fix test failure

0c2cd8b

julienf-unity approved these changes Nov 2, 2021

View reviewed changes

PaulDemeulenaere self-requested a review November 2, 2021 10:51

PaulDemeulenaere approved these changes Nov 2, 2021

View reviewed changes

ludovic-theobald added 2 commits November 3, 2021 10:38

Fix metal refs

8967cbc

exclude 16_MeshParticles in XR

39bad8e

PaulDemeulenaere reviewed Nov 3, 2021

View reviewed changes

...rojects/VisualEffectGraph_HDRP/Assets/AllTests/VFXTests/GraphicsTests/16_MeshParticles.unity Show resolved Hide resolved

ludovic-theobald merged commit f4079dc into master Nov 4, 2021

ludovic-theobald deleted the vfx/feature/new-sorting-keys branch November 4, 2021 07:24

PaulDemeulenaere added a commit that referenced this pull request Nov 4, 2021

*Update All VFX

8065aea

Needed after merge of #5125

PaulDemeulenaere mentioned this pull request Nov 4, 2021

[VFX] Staging to Master #6172

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[VFX] Additional Sorting Keys [22.1] #5125

[VFX] Additional Sorting Keys [22.1] #5125

Uh oh!

ludovic-theobald commented Jul 13, 2021 •

edited

Loading

Uh oh!

github-actions bot commented Jul 13, 2021 •

edited

Loading

Uh oh!

VladNeykov commented Jul 27, 2021

Uh oh!

julienf-unity left a comment

Uh oh!

julienf-unity Aug 16, 2021

Uh oh!

Uh oh!

ludovic-theobald commented Oct 4, 2021

Uh oh!

Vic-Cooper left a comment

Uh oh!

julienf-unity Oct 27, 2021

Uh oh!

julienf-unity Oct 27, 2021

Uh oh!

julienf-unity Oct 27, 2021

Uh oh!

PaulDemeulenaere left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[VFX] Additional Sorting Keys [22.1] #5125

[VFX] Additional Sorting Keys [22.1] #5125

Uh oh!

Conversation

ludovic-theobald commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

[VFX] Additional Sorting Keys

Checklist for PR maker

Purpose of this PR

JIRA story

Link to C++ PR : https://ono.unity3d.com/unity/unity/pull-request/130921/_/graphics/vfx/feature/new-sortkeys

Testing status

Comments to reviewers

Uh oh!

github-actions bot commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VladNeykov commented Jul 27, 2021

Uh oh!

julienf-unity left a comment

Choose a reason for hiding this comment

Uh oh!

julienf-unity Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ludovic-theobald commented Oct 4, 2021

Uh oh!

Vic-Cooper left a comment

Choose a reason for hiding this comment

Uh oh!

julienf-unity Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

julienf-unity Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

julienf-unity Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

PaulDemeulenaere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ludovic-theobald commented Jul 13, 2021 •

edited

Loading

github-actions bot commented Jul 13, 2021 •

edited

Loading