Add support for GPU instancing of mesh particle systems #904

ghost · 2020-06-15T15:17:39Z

Please read

PR workflow guidelines

SRP ABV will start automatically on Yamato when you open your PR
Changes to docs and md files will not trigger ABV jobs
Consider making use of draft PRs if you are not 100% sure that your PR is ready for review
ABV will restart if you add a new commit to a branch with an open PR (hence why you should consider using draft PRs)
Adding [skip ci] (case insensitive) to the title of PRs will stop any jobs being trigger automatically - you will need to open Yamato and find your branch to run ABV
You can also add [skip ci] to commit messages to prevent CI from running on that push
Add [cancel old ci] to your commit message if you've made changes you want to test and no longer need the previous jobs

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.
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

Adds support for GPU instanced mesh particles. Without this, the built-in renderer is far more efficient on some devices.

Testing status

Manual Tests: What did you do?

Opened test project + Run graphic tests locally
Built a player
Checked new UI names with UX convention
Tested UI multi-edition + Undo/Redo + Prefab overrides + Alignment in Preset
C# and shader warnings (supress shader cache to see them)
Checked new resources path for the reloader (in developer mode, you have a button at end of resources that check the paths)
Other:

Automated Tests: What did you setup? (Add a screenshot or the reference image of the test please)

Yamato: (Select your branch):
https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics

Any test projects to go with this to help reviewers?

Comments to reviewers

Requires https://ono.unity3d.com/unity/unity/pull-request/108978/_/core/particles-expose-shadergui-api-for-urp, which hasn't quite landed in trunk at the time off writing this.

github-actions

Welcome to the Unity SRP repo!
Please make sure to fill out the PR template as best you can to give reviewers as much information as possible.
If you have any questions (and you are a Unity employee) go to "#devs-renderpipe"

eh-unity · 2020-06-30T11:49:06Z

Changelog entry is missing. Also would be nice if the PR description would be finished.

ellioman

Looks good!
Two things:

I'd really like a test scene in the URP Test project for this
Changelog missing

If you need help with 1. then I or someone from the URP team can easily help :)

eh-unity

Ugh. I added these comments days ago, but I always forget to "commit" reviews comments to make them visible. I'm just too used to other systems in the past, that just show the comment immediately after adding.

com.unity.render-pipelines.universal/Editor/ShaderGUI/ParticleGUI.cs

eh-unity · 2020-06-30T11:14:24Z

com.unity.render-pipelines.universal/ShaderLibrary/Particles.hlsl

+#if !defined(UNITY_PARTICLE_INSTANCE_DATA_NO_COLOR)
+    UNITY_PARTICLE_INSTANCE_DATA data = unity_ParticleInstanceData[unity_InstanceID];
+    color = lerp(half4(1.0, 1.0, 1.0, 1.0), color, unity_ParticleUseMeshColors);
+    color *= float4(data.color & 255, (data.color >> 8) & 255, (data.color >> 16) & 255, (data.color >> 24) & 255) * (1.0 / 255);


We probably should and int/float4 packing/unpacking functions. In core/ShaderLibarary/Packing.hlsl there are similar functions (e.g. UnpackFromR11G11B10f). Consider adding pack/unpack for this case too.

I suppose this is the same case as the matrix invert. I think it would be good to add a comment/TODO to replace with library implementation when available.

ah, i missed this comment when i read over the PR feedback, sorry. i think this one is actually more clear-cut than the matrix one - i don't see a reason why i cant move it to the Packing file. I'll sort that out :)

eh-unity · 2020-06-30T11:30:12Z

com.unity.render-pipelines.universal/ShaderLibrary/Particles.hlsl

+        float index0 = floor(sheetIndex);
+        float vIdx0 = floor(index0 / numTilesX);
+        float uIdx0 = floor(index0 - vIdx0 * numTilesX);
+        float2 offset0 = float2(uIdx0 * animScale.x, (1.0 - animScale.y) - vIdx0 * animScale.y);


Seems like a Y-flip. If that's the case, then it would be good to have a comment why it's done (and if the comment has a word flip in it then it's searchable if someone is tracking down a chain of flips :) )

Honest answer: I dunno - it's copied from the built-in instancing code which is coped from the built-in cpu code which is really old. Something about UV space, I suppose :)

That's a bit unfortunate. Maybe at least add a comment that it's from built-in as is and it looks like upside-down flip. It wouldn't hurt to mention the flip in the comment (for searchability).
Ideally all flip chains should be removed/minimized, but that probably isn't realistic at this point and certainly not in the scope of this PR.

eh-unity · 2020-06-30T11:44:51Z

com.unity.render-pipelines.universal/ShaderLibrary/ParticlesInstancing.hlsl

+
+    // inverse transform matrix
+    float3x3 w2oRotation;
+    w2oRotation[0] = objectToWorld[1].yzx * objectToWorld[2].zxy - objectToWorld[1].zxy * objectToWorld[2].yzx;


If this is just rotation, would just transpose be enough? That's inverse for rotations, but scales can be problem.
In any case we should have a function for generic matrix inverse, but I couldn't find any. Maybe use this to make generic matrix inverses for mat4,3,2 etc. While it's slow, it's sometimes necessary and useful for debugging spaces too.

If this is just rotation, would just transpose be enough? That's inverse for rotations, but scales can be problem.
In any case we should have a function for generic matrix inverse, but I couldn't find any. Maybe use this to make generic matrix inverses for mat4,3,2 etc. While it's slow, it's sometimes necessary and useful for debugging spaces too.

It's not just rotation :)

I am super hesitant to add any code that is unused (as the mat4 and mat2 variants would be). It's best done at a time when they can be tested as being correct IMO. (i.e. when there is a use case)

If you would like me to move this code in a function called "invertMatrix" or similar, let me know the name and file you'd like, thanks :) (This is my first -and perhaps only- URP PR, so I don't know my way around your shader library)

OK. I can agree that adding those isn't in the scope of this PR.
Perhaps add a todo comment to replace with a library implementation if/when available, so the info is not lost.
Also if it's not just rotation then perhaps the variable name 'w2oRotation' could be more clear.

Opted for worldToObject3x3

VladNeykov · 2020-07-10T11:18:18Z

Hi @richardkettlewell , started on this, but getting errors like:

'ShaderUtil' does not contain a definition for 'HasProceduralInstancing'
'ParticleSystemRenderer' does not contain a definition for 'supportsMeshInstancing'

Is there a custom binary branch I should be on? I'm on 2020.2.0a18.2359 from 2 days ago.

ghost · 2020-07-13T11:07:26Z

Hi @richardkettlewell , started on this, but getting errors like:
'ShaderUtil' does not contain a definition for 'HasProceduralInstancing'
'ParticleSystemRenderer' does not contain a definition for 'supportsMeshInstancing'
Is there a custom binary branch I should be on? I'm on 2020.2.0a18.2359 from 2 days ago.

Yes sorry! https://ono.unity3d.com/unity/unity/pull-request/108978/_/core/particles-expose-shadergui-api-for-urp

…move color unpacking to a util function

ellioman

Approving as my comments have been addressed :)

phi-lira · 2020-07-15T08:00:35Z

@Unity-Technologies/gfx-qa-foundation @VladNeykov any update on this one?

VladNeykov

Occasional crash and visual glitches, very hard to repro, but should probably be addressed in this PR:
https://docs.google.com/document/d/16ez5yDhtTIEXy3rXGMTrZMmCcfJITZ0xus2HbqvnjvE/edit?usp=sharing

(will keep trying to find reliable repro, but @richardkettlewell if anything here rings a bell, maybe you can beat me to it)

…cing # Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md

VladNeykov

LGTM; some outstanding issues, but as discussed, they are not introduced by this PR.

ghost · 2020-07-16T13:25:01Z

Looks good!
Two things:

I'd really like a test scene in the URP Test project for this

Changelog missing

If you need help with 1. then I or someone from the URP team can easily help :)

Done and done :)

Add support for GPU instancing of mesh particle systems

17b7a66

ghost self-requested a review as a code owner June 15, 2020 15:17

ghost requested a review from eh-unity June 15, 2020 15:17

github-actions bot added the universal label Jun 15, 2020

github-actions bot reviewed Jun 15, 2020

View reviewed changes

ghost assigned phi-lira Jun 15, 2020

ellioman requested review from a team and ellioman June 30, 2020 09:55

richardk-u3d added 2 commits July 2, 2020 12:14

Fix GPU instancing UI

bfa1e14

Hide GPU Instancing field for particle shaders - it is irrelevant

9539d39

ellioman suggested changes Jul 3, 2020

View reviewed changes

VladNeykov self-requested a review July 7, 2020 08:47

Merge branch 'master' into universal/particle-shader-instancing

890cc25

eh-unity reviewed Jul 9, 2020

View reviewed changes

richardk-u3d added 4 commits July 9, 2020 14:23

[particles] add particle instancing gfx test

7a2d6ca

Remove unused meta

0c451f7

[particles] Fix flipblook blending

d0d7cd6

[particles] Changelog entry for instanced mesh particles

c5c7fca

[Skip CI] [particles] Add some code comments, rename some variables, …

d95b98a

…move color unpacking to a util function

ellioman approved these changes Jul 14, 2020

View reviewed changes

VladNeykov requested changes Jul 15, 2020

View reviewed changes

[Skip CI] Merge branch 'master' into universal/particle-shader-instan…

97e0af3

…cing # Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md

VladNeykov approved these changes Jul 15, 2020

View reviewed changes

richardk-u3d added 2 commits July 16, 2020 11:23

[particles] Add DX11 gfx test image for particle instancing

16b8491

New particle images

634efec

ghost closed this Jul 16, 2020

ghost reopened this Jul 16, 2020

stramit merged commit 7847a5c into master Jul 17, 2020

stramit deleted the universal/particle-shader-instancing branch July 17, 2020 09:31

Add support for GPU instancing of mesh particle systems #904

Add support for GPU instancing of mesh particle systems #904

Uh oh!

Conversation

ghost commented Jun 15, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please read

Checklist for PR maker

Purpose of this PR

Adds support for GPU instanced mesh particles. Without this, the built-in renderer is far more efficient on some devices.

Testing status

Comments to reviewers

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

eh-unity commented Jun 30, 2020

Uh oh!

ellioman left a comment

Choose a reason for hiding this comment

Uh oh!

eh-unity left a comment

Choose a reason for hiding this comment

Uh oh!

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!

eh-unity Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ghost Jul 13, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VladNeykov commented Jul 10, 2020

Uh oh!

ghost commented Jul 13, 2020

Uh oh!

ellioman left a comment

Choose a reason for hiding this comment

Uh oh!

phi-lira commented Jul 15, 2020

Uh oh!

VladNeykov left a comment

Choose a reason for hiding this comment

Uh oh!

VladNeykov left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ghost commented Jun 15, 2020 •

edited by ghost

Loading

eh-unity Jul 10, 2020 •

edited

Loading

ghost Jul 13, 2020 •

edited by ghost

Loading