Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added an emissive as forward pass when the lit is in deferred mode + Material update #3430

Merged
merged 75 commits into from
Feb 18, 2021

Conversation

sebastienlagarde
Copy link
Collaborator

@sebastienlagarde sebastienlagarde commented Feb 9, 2021

Caution: This PR contain tons of Material, to review think to not expand Material

Purpose of this PR

In order to properly support APV and SSGI(RTGI). We need to be able to not impact the emissive when in deferred mode. To do so, we added the option (Force Forward) to have the emissive rendered in a separate forward pass (ForwardEmissiveForDeferred) only for object that have non null (default value SG or shader) or with a branch plugged in (SG).

  • This have effect only possible when the lit model is in deferred
  • The new pass (ForwardEmissiveForDeferred) is not generated for shader graph if the emissive value is black or nothing is plugged in the master node on the emission port.
  • The pass is disabled on non shader graph shader for material that have null emissive values.

The extra pass is stripped out in the player if the Lit mode is forward

For reviewer: This PR also remove the EmissiveOpaque pass, it is now merge with the regular ForwardOpaque pass to save a Drawrenderer (and thus a CPU pass).

Testing status
Added a Yamato test 2011 covering all usage types.
image

  • Tested UX of shader graph / Material associated. Checked that the checkbox correctly display
  • Checked the generated code of shader graph to see that the code is correctly removed
  • Checked in RenderDoc that the material render in the correct pass.
  • Checking the emissive override mode (SG Unlit don't work but it is not related to this PR), and emissive only lighting mode.

There is now an option "Force Emissive Forward" available on Lit shader and also on Material created from Lit Master node that have Emission to control the behavior:

image

A script have been added to tag autoamtically a selection of Material
"Edit/Render Pipeline/HD Render Pipeline/Enable Force Forward Emissive on Selected Lit Material".

For QA:
This PR introduce a new pass to render Emissive Color in a new pass - ForwardEmissiveForDeferred - but only for Lit (SG Lit, LayeredLit, LitTessellation, LayeredLitTessellation, Lit) Material with a Surface Type Opaque.
The Emissive contribution of a Material will be rendering into the Lighting Buffer during the GBuffer pass or in an extra Emissive Color pass which is after all the other lighting based on the "Force Forward Emissive" checkbox on Material or SG.
This change of rendering order only apply for object render in the deferred pass; Transparent Material or other Material (hair, stacklit, fabric, eye, axf, unlit) aren't affected.

Plan test:

When enabling "Force Forward Emissive" in a Material with Lit emissive objects, we should get the same visual result. However there is a difference, now the SSAO algorithm will not affect the emissive and in addition, if two emissive object are on top of each other, they will contribute twice (against one without the change). The new pass can be check in a RenderDoc capture or the FrameDebugger. If Lit Material don't have a Emissive contribution, then nothing should be render in this extra pass.
It can be validated that it work correctly with the SSGI effect. SSGI effect by default remove emissive contribution of object.

Be sure also to check the player with Material which used or not the Force Forward option to validate that the stipper work.


Testing status

Describe what manual/automated tests were performed for this PR


Comments to reviewers

Notes for the reviewers you have assigned.

Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

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

What is working as expected :

  • Documentation ✔️
  • Automation ✔️
  • Checked that null emissive in material or black emissive in SG won't trigger the pass ✔️
  • Classic Lit / SG Lit ✔️
  • Lit Variants ✔️ (Layered Lit.. etc)
  • Changing Forward / Deferred at runtime ✔️
  • Emissive material with SSGI and ith flag Receive SSGI with Force Forward Emissive gets emissive contribution back ✔️
  • Stacking n emissive materials with Force Forward Emissive contibute n time to the emission ✔️
  • Unssuported shader (AxF, eye, Hair.. etc) don't have option and look the same ✔️
  • Render pipeline menu item Enable in Scene / Disable in scene ✔️
  • DX11/DX12/Vulkan (Windows) ✔️

What I couldn't test "yet" :

  • Couldn't test with Light Probe because I crash everytime I try to bake them for now
  • Performance (not HDProfileID AFAIK)
  • Other platforms (Linux / Metal) but yamato should take care of it
  • Should we test on consoles ?

Issues found :

  • Render pipeline menu item Enable in selection. It's working but it throw errors in the console (Enable/Disable in scene works well)
    ForceEmissive_MenuSelection_Error

  • When building with Lit Shader mode at Both or Deferred Lit shader lose the new pass

In editor (all material are emissive with receive SSGI; First and Third coloumn have force forward emissive ON)
RTGI_ForceForwardEmissive_Editor

In player
RTGI_ForceForwardEmissive

What can/may be improved :

  • Is there a way to "hide" the Force Forward Emissive Advanced Option if the Surface Type of the Lit material is set to Transparent ? (The same way unsupported shaders, AxF, Hair.. etc won't show the option) ?
  • Wanted to check performance but couldn't find a "ForwardEmissiveForDeferred" marker in HDProfileId.cs, did we forgot about it or is it not possible to profile for some reasons ?
  • Fix typo in tooltip :
    "When in Lit shader mode: deferred. It forceS the emissive part of the material to be renderED into an additional forward pass. This can improve quality and solve artEfactS when using SSGI but have an additional CPU and GPU cost"
  • Do we want to add a frame setting for this pass ?
  • Can we rename item in the menu to be consistent with other menu item like this :
    • "Enable Force Forward Emissive on Selected Materials"
    • "Enable Force Forward Emissive on Scene Materials"
    • "Disable Force Forward Emissive on Scene Materials"

@sebastienlagarde
Copy link
Collaborator Author

Wanted to check performance but couldn't find a "ForwardEmissiveForDeferred" marker in HDProfileId.cs, did we forgot about it or is it not possible to profile for some reasons ?

you can't, the forward emissive is now merge inside the forward pass, so no marker. To validate performance you need to check the whole pass, but you will need lot of object to see a difference (on the CPU manily).

Can we rename item in the menu to be consistent with other menu item like this :
will do following Alex change

Do we want to add a frame setting for this pass ?
nope, we have get a whole slack discussion about it and the ForceEmissive isn't a framesettings

will address other comment.

I will fix the player first:

When building with Lit Shader mode at Both or Deferred Lit shader lose the new pass
it mean I have fucked it, and then we can do a run on console for sanity check.

Thanks for the coverage!

@Vic-Cooper
Copy link
Contributor

@sebastienlagarde Got one outstanding question for the docs :

Thanks for the explanation! One other thing, when you say "Same usage for the new Adaptive probe volumes." does that mean Adaptive probe volumes has the same artifact or does it mean you can use the checkbox in the same way with Adaptive probe volumes too?

@sebastienlagarde
Copy link
Collaborator Author

@sebastienlagarde Got one outstanding question for the docs :

Thanks for the explanation! One other thing, when you say "Same usage for the new Adaptive probe volumes." does that mean Adaptive probe volumes has the same artifact or does it mean you can use the checkbox in the same way with Adaptive probe volumes too?

does that mean Adaptive probe volumes has the same artifac
This.
Adapative probe volumes and SSGI/RTGI both result in lossing the emissive material contribution. This new feature fix it for all those features.

@sebastienlagarde sebastienlagarde marked this pull request as ready for review February 17, 2021 19:01
@sebastienlagarde
Copy link
Collaborator Author

@Vic-Cooper @remi-chapelain : all (possible) feedback adressed and error fixed. PR ready for review/merging now

still wait for this one I reply above

|@sebastienlagarde Got one outstanding question for the docs :

thanks!

@Vic-Cooper
Copy link
Contributor

Resolved outstanding docs question in commit 80f9aa0. Docs review complete.

Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

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

Made another pass :

What is working as expected :

  • Building a player with force emissive material checked ✔️
  • Multi-materials objects with mix of Force Emissive / Transparent / Regular Emissive Opaque ✔️
  • Force Forward Emissive is now only visible is compatible opaque materials ✔️

What still need fixing

  • Tooltip have been updated but uppercase letters to show what needs fixing have been kept.
    image

  • With latest commit, we lost a bunch of menu item in the Edit > Render Pipeline
    Before :
    image
    After :
    image

  • About the two menu item "disable/enable all materials", your changes are making me doubt.
    If, as I thought, only the material (as the code suggests) in the scene are impacted, adding a "Scene" in the label would be great !
    image

  • About the menu "Force on Selected Materials" , I just noticed (looking at the code) it's not only working with selected material in project, if I click the menu with selected renderer in the hierarchy, it will create new instance of these gameobject's materials with force emissive on. Also, it spam an error in console related to it.
    image
    So I suggest either :

    • Avoid creating instance of materials when selecting gameobjects in the hierarchy with this feature
    • Disable the possibility to Force Forward Emissive with selected renderer in the hierarchy

@sebastienlagarde
Copy link
Collaborator Author

Tooltip have been updated but uppercase letters to show what needs fixing have been kept.
I am stupid :P

If, as I thought, only the material (as the code suggests) in the scene are impacted, adding a "Scene" in the label would be great
I am following the new convention setup in Alex PRs

Disable the possibility to Force Forward Emissive with selected renderer in the hierarchy
Done

For the menu I have forget to remove the RenderPipeline level, here is the new menu

image

as you call see everywhere it is "scene" have been replace by all.

Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

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

We're getting there on the tooltip, one char left and we're good :)
image

As for the "All" / "Scene" debate, It's good that we're consistent and I won't block the PR for this but I still think using "All Materials" instead of "All Scene Materials" is confusing and can be mistaken for "All Project Materials".
There's also the "Convert Scene Terrains to HDRP Terrains", I prefer it this way, like it is now, but if we replace "scene" by "all", why keeping scene here ?
Especially with the just above "Convert All Built-in Materials to HDRP" that converts all the project materials. How are we supposed to understand the difference?

image

@sebastienlagarde
Copy link
Collaborator Author

We're getting there on the tooltip, one char left and we're good :)
I am feeling so shame by this :)

Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

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

Still have mixed feeling for the All Materials vs All Scene Materials but won't block the PR for it !
All the rest of the issues/concerns have been adressed !
Approving. ✔️

@sebastienlagarde sebastienlagarde merged commit 96e4258 into master Feb 18, 2021
@sebastienlagarde sebastienlagarde deleted the HDRP/foward-emissive-opaque-material branch February 18, 2021 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants