Skip to content

[Shader Graph] Master Stacks [Skip CI]#451

Closed
elizabeth-legros wants to merge 1879 commits into
masterfrom
sg/master-stack
Closed

[Shader Graph] Master Stacks [Skip CI]#451
elizabeth-legros wants to merge 1879 commits into
masterfrom
sg/master-stack

Conversation

@elizabeth-legros
Copy link
Copy Markdown
Contributor

@elizabeth-legros elizabeth-legros commented May 13, 2020

alt text

Purpose of this PR

Problem

Currently, the master node definition is too rigid and defined via leaky abstractions. This gives the the following problems:

  • Large list of individual master nodes. Changing master node means reconnecting all edges.
  • Many HDRP features require HDRP specific master nodes. This prevents the shader from compiling in URP.
  • Users cannot define new master nodes without internal Shader Graph access.
  • Ports from different stages are mixed into the same master node, this is confusing for users.

There are other problems involved but these are the main concerns because they seriously inhibit our ability to open up our API to customers (internal right now, external later). As Shader Graph adoption grows, this problem continues to get worse.

Solution

We propose a solution where, instead of providing a collection of master nodes, we provide a single, extensible stack. Users will enable and disable shader features, per render pipeline, in the Inspector. These features provide input ports on the stack, where the user then connects graph edges to provide data for those features. Different render pipelines then build a shader using the input data they support, based on the pipeline configuration. This ensures all features are available, but the shader is always in a valid state.

Goals

  • All features available from any render pipeline, still compiles on all render pipelines by ignoring unsupported features.
  • UX concepts applied meaningfully, and in line with other graph products.
  • Extensible design language for future feature work

What do these mean in practice?

  • No more master node explosion, all features should be available from one stack
  • All HDRP features must be available, and this shader should always have a fallback on URP
  • Better parity with VFX and VS, using the same visual language as their stack implementations
  • Better visual distinction between vertex and fragment stages.
  • Lay the foundations for…
    • Custom interpolators
    • Public “master node API”
    • Layering/custom passes

TODO

  • Fix remaining red tests
  • Fix known issues
  • Add new tests (edit/graphics)
  • Test for performance regressions in graph
  • Add changelog entries

Testing status

PR Template:

  • 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)

General Graph usage

  • Fill this out...

Upgrade Tests:

  • Upgrading V0 > V2
  • Upgrading V1 > V2
  • All master nodes
    • Various ports connected
    • Various settings enabled
    • Ports that require settings and connected
    • Default port values changed
  • PBR specular/metallic (only add one block)
  • Alpha clip upgraded from slot logic > bool (PBR/Unlit)
    • Alpha clip value changed
    • Alpha clip connected
  • Multiple master nodes
  • Master position > Context position
  • Sprite masters result in SpriteColor block (legacy)
    • If BaseColor or Alpha are added SpriteColor is ignored
    • SpriteColor cannot be readded
  • AxF shaders in URP package
  • Upgrading VFX SGs used in VFX graphs

Automated Tests:

  • To add. See TODO checklist.

Any test projects to go with this to help reviewers?
Upgrade graphs can be found at https://easyupload.io/rbhlsn (pw: unity)

Test document:
https://docs.google.com/document/d/1O_wnqsQ8zsnsl-QJ83byZJbLk5O8Od740LRASyj_VP0/edit?usp=sharing

Comments to reviewers

elizabeth-legros and others added 30 commits April 16, 2020 14:25
* fix warning

* changelog

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
* Exposure weighted by texture

* miss one in the renaming

* Doc update

* changelog

* doc update

* fix issue when disabling override
* Saving 10% off the PrepareLightForGPU

* 18% decrease in cost with this

* Around 12% cost shaving off GetLightData

* Around 6.5% win here

* Faster View matrix flip

* Missing *=-1

* About 7% win in preprocess light data

* Another small batch

* Small cleanup for first optimization pass

* Tiny bit more cleanup

* Make UpdateShadowRequests 12% cheaper

* Address review points

* A bit less than 20% win in ExtractPointLightData

* Add comment

* changelog

* 25% reduction of ExtractPointLightData

* 18% win in UpdateShadowRequest

* Another around 11% win for UpdateShadowRequest

* Move matrix multiply utils to core

* less lighttype computation

* Ooops

* fixup changelog

* Proper merge

* Missing leftovers .

* I'll manage to submit it all one day...

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
* Add option to use Base UV Mapping for emission

* Graphic test and doc

* Changelog

* Screenshots

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
* HDRP compositor tool, first code drop

documentation typo, move everything in compositor namespace

documentation typo

bugfix: dynamically add/remove properties when shadergraph changes

UI bugfix: properly compute height of UI elements

Missing tooltips, button to remove the compositor

toggle layers now works when player is paused

add chroma keying in the compositor test

Changelog, workflow bugfix

Use Unity's built'in icons

AOV bugfixes

AOV tooltip was wrong

bugfix: disabling the compositor now disables all compositor cameras

Proper monitoring of shadergraph changes

Updated test image + minor changes

Use better names for internal cameras

Bugfix: selecting the same res for a layer

Bugfix: deleting in-use texture layers from shadergraph now works

Bugfix: setting AOV to none now works properly

Use the term SubLayer in the UI

Improve AOV tooltip

Camera properties in layers are properly refreshed

UI: Move checkbox for overrides on the left side of the label

Remove default compositor camera when also deleting the compositor

Bugfixes: no orphans (delete all children when removing a layer), saving the layer list now works again

Hdrp/docs/compositor (#6013)

* Added docs review

* Changed fodler for images

Bugfixe: deleting layers

Bugfix: reordering layers

Bugfix: creating a new compositor fails

Remove redundant check

Fix wrong tooltip

Bugfix when deleting camera stack layers

Bugfix: handle window resize

Bugfix: sub-layers without parent when re-ordering, preview remains visible for disabled layers

Bugfix: more robust re-ordering (also moves children)

Bugfix: mark scene dirty when adding a compositor

Bugfix: display warning when alpha is configured properly

Change video layer icon

Bugfix: adding a layer filter does not reset the UI state

Bugfix: do not show composition properties that are marked with hide-in-inspector

Better / more clean layer injection implementation

minor code clean up

Clear color mode should only be editable on the first layer of a stack

Bugfix: setup of custom render was not always called

Bugfix: make sure additional camera data always exist in the layer/compositor camera

Change test id to 9800

Fix typos

mark scene dirty when removing or disabling the compositor

Default composition shadergraph and profile are now cloned

Serialized objects are now properly updated

minor code change: range checks

bugfix: error message appears at beginning

remove layer list from the asset file

add default camera layer when creating the compositor

replace getter function with c-sharp property

update asset in unit test

* move compositor unit test images

* remove leftover code

* review feedback #1

* review feedback 2

* review feedback - custom clear shader

* fix variable names that don't affect serialization

* fix serialized variable names

* bugfix: image layers and camera stacking

* use enum utility functions

* Use multicompile in motion blur + bug fix

* Make some variables private

* Update Compositor-User-Guide.md

* Review feedback

* fix muticompile for motion blur without alpha

* Update to new constant buffer / shadergraph api

* Player test image for the compositor

* Properly fill camera entries for global constant buffer

* Fix after rebase / function rename

* Make bg red for better debugging qnd cqtch unity exception

* Disable compositor test until we fix it on yamato

Co-authored-by: Lewis Jordan <lewis.jordan@hotmail.co.uk>
Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
* Fixed PBR shader rendering in deferred

* Updated changelog
* Multiple Importance Sampling for HDRI Sky (Ray Generation)

* backup

* Multiple helpers

* Add RTHandleDeleter (delay in K frame the Release of a RTHandle)
* Fix GPU Operation when texture are too small
* Helper to manager Mariginal Texture in general (Tex2D, Tex2DArray, CubeMap, CubemapArray), ...
* Helper manager to generate one marginal per slice mip (one per frame)

* backup

* Add SampleCubemapProjectionNode (Project cubemap to Geometry)

* delete useless files

* update

* Cube To LatLong for CubeArray

* Functional generation

* Integration on ray tracer step

* Store HDRI Integral

* .

* Integrate HDRI Sky

* Update Importance Sampling

* Update Algorithm with Hemisphere Path

* Backup

* Update merge

* Update Merge 001

* Update Merge 002

* Update Merge 003

* Update Merge 004

* Update Merge 005

* Update Merge 006

* Update Merge 007

* Cleanup

* Update Integration of HDRISky Upper Hemisphere

* Remove useless dump images on disk

* Fix change log issue

* backup

* Backup

* Pre-Clean

* Pre-Clean

* Clean Up - remove file

* Remove Debug Code

* Update

* Update ChangeLog

* Clean Up

* update

* Clean up

* Remove useless texture

* Fix CGAlloc + Default Litetime of a RTHandle on the "RTHandleDeleter"

* CleanUp

* remove useless comment

* Add variable (Atmospherical) + Rename/Comment

* Move parameter to CS, Clean & move to private RTHandleDeleter
#94)

* - Replaced the DIFFUSE_LIGHTING_ONLY multicompile by a uniform.
- Removed the dynamic lightmap multicompile.
- Remove the LOD cross fade multi compile for ray tracing.
- Added a ray tracing mode option in the HDRP asset that allows to override and shader stripping.

* fix merge

* Update LayeredLit.shader

* Update LayeredLit.shader

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
…jects. (#93)

* - Avoid building the mip chain a second time for SSR for transparent objects.

* Update ScreenSpaceReflections.compute

* fix merge issue

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
* Adaptive sampling works

* Compute the PDF

* URL

* Fix stuff up (except for profile preview)

* Attempt to fix previews

* Fix transmission

* Support quality levels

* Clean shader

* Remove 2 kernels

* PURGE

* Automatically adjust the number of samples at runtime

* Changelog

* Spaaaaace

* Add comment

* Spaaaaaace

* Rename

* Update ref img 1215, 1216, 1217

* Revert "Update ref img 1215, 1216, 1217"

This reverts commit 9aeb1fee054bc0e8a24b76525eb4af5dc2316088.

* Robust filter radius

* Solve the NaN problem

* Save 1x VGPR

* Update SSS defaults

* Add migration steps

* Volume resolution WIP

* Shorter names FTW

* Comment

* Dynamic slice count works

* Dynamic res works

* Changelog

* Stretch the limits

* Fix bug (order matters)

* Comments

* Remove volumetric quality presets

* Changelog

* Reorder

* Solve the mystery of RTs going NULL

* More robust bad history detection

* Update test scenes for High Quality

* More bug fixes

* More accurate comment

* Do not reinvent the wheel

* Fix broken test 1351

* Attempt to migrate HQ SSS settings

* Remove all branches

* Optimize a bit

* 7x WF

* Fix the rounding error

* Fix the rounding error

* Remove garbage allocation

* Fix typos

* Copy the SSS value

* Revert HDRP asset changes

* Fix default UI value

* Pass the right asset

* Update ref img 1215

* update reference screenshots

* Update CHANGELOG.md

* Update CHANGELOG.md

* Switch to RTHandles

* Rename textures used by the fog filtering pass

* Fix reprojection

* Fix shader warning + typo+

* fix another shader warning

Co-authored-by: EvgeniiG <7ee2cc898cca1b5fc49df740c2081dfc681e0a28>
Co-authored-by: Sebastien Lagarde <sebastien@unity3d.com>
* Revert "Hdrp/runtimetests/ps4 fix (#129)"

This reverts commit 6fa52ab.

* Update runtime test to use way less memory to be able to run on console

* Fixed screenshots and test
if (!supportedByMasterNode)
ps.Add(new HelpBoxRow(MessageType.Warning), (row) => row.Add(new Label("The current master node does not support Virtual Texturing, this node will do regular 2D sampling.")));
}
//TODO: Fix the fact bool passing that happened with master nodes to now happen with targets/subtargets
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with no idea of master nodes now, need to probably put this bool flag on either targets or subtargets

@ghost
Copy link
Copy Markdown

ghost commented May 14, 2020

elizabeth-legros and others added 4 commits May 14, 2020 13:48
…tack

# Conflicts:
#	com.unity.shadergraph/Editor/Data/Nodes/AbstractMaterialNode.cs
#	com.unity.shadergraph/Editor/Drawing/Views/GraphEditorView.cs
#	com.unity.shadergraph/Editor/Drawing/Views/PropertyNodeView.cs
@alelievr alelievr closed this May 15, 2020
@alelievr alelievr reopened this May 15, 2020
@alelievr
Copy link
Copy Markdown
Contributor

Sorry, misclick while my PC froze :(

@elizabeth-legros
Copy link
Copy Markdown
Contributor Author

Misunderstood when branch off was happening, undoing previous master merge

@elizabeth-legros elizabeth-legros deleted the sg/master-stack branch May 15, 2020 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.