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

Universal/Enlighten Realtime GI support #3386

Merged
merged 96 commits into from Apr 15, 2021
Merged

Conversation

KEngelstoft
Copy link
Contributor

@KEngelstoft KEngelstoft commented Feb 4, 2021

Purpose of this PR

Add Enlighten Realtime GI support to Universal. Makes it easier to upgrade from builtin render pipeline.

Testing status

Tech art validation pass completed. Details here https://jira.unity3d.com/browse/GFXGI-401.
SRP batcher has been tested and fixed.

Automated test coverage has been added in the following scenes:
150_Lighting_EnlightenEmission
151_Lighting_EnlightenLights
152_Lighting_EnlightenEnvironment
153_Lighting_EnlightenTerrain
Forward and deferred rendering path is covered by the tests.
Tests are covering Editor and Standalone Player on desktops and mobile platforms across DX11, DX12, Vulkan, Metal and GLES3.0.

Realtime GI has been disabled in all other scenes (separate PR, already landed in master).

Comments to reviewers

Material inspector has been streamlined to work like the build in one (user can now choose between baked and realtime GI emission).

Documentation has been updated.

Jira task https://jira.unity3d.com/browse/GFXGI-394

Copy link

@radishface radishface left a comment

Choose a reason for hiding this comment

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

I have finished an extensive review pass on the feature. For more details on testing methodology, please refer to the dedicated feature test document.

No major issues were found while testing. Great work! Approved.

@phi-lira phi-lira removed the request for review from pbbastian April 8, 2021 06:57
@KEngelstoft KEngelstoft marked this pull request as ready for review April 9, 2021 08:31
@KEngelstoft KEngelstoft requested a review from a team as a code owner April 9, 2021 08:31
@KEngelstoft
Copy link
Contributor Author

Minor things. PR looks great overall. Thanks for fixing the unity_LightmapIndex and the TerrainDetailLit shader.

Heads up, the test images will most likely have to be remade as we are landing split project testing work.

Template images are up to date now after dealing with the merge of the project split.

Copy link
Contributor

@phi-lira phi-lira left a comment

Choose a reason for hiding this comment

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

Approved, but it seems there is a regression in terrain and ambient light. That should be fixed before merging.

Comment on lines -117 to -118
half3 bakedGI = SampleLightmap(input.LightmapUV, half3(0.0, 1.0, 0.0));

Copy link
Contributor

Choose a reason for hiding this comment

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

How are we getting ambient skylight / SH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up bringing the code back here 95e5f15 but in a more generic shape so SH sampling is possible for terrain details.

@lukaschod
Copy link
Contributor

Is mobile manual testing planned? I have concerns about additional varyings in shadergraph as they are not handling packing that well at it might result in more interpolators now.

StructFields.Varyings.positionWS,
StructFields.Varyings.normalWS,
StructFields.Varyings.tangentWS, // needed for vertex lighting
StructFields.Varyings.viewDirectionWS,
UniversalStructFields.Varyings.lightmapUV,
UniversalStructFields.Varyings.staticLightmapUV,
UniversalStructFields.Varyings.dynamicLightmapUV,
Copy link
Contributor

Choose a reason for hiding this comment

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

I have concerns here regarding the shadergraph as they do not handle conditional interpolator packing that well. So it might result more interpolators even with disabled varyings. I think we need mobile test pass to be sure on that

Copy link
Contributor

@lukaschod lukaschod left a comment

Choose a reason for hiding this comment

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

@felipe dmed me that this had mobile qa pass, approving in that case

Before, only lightmaps were sampled but SH makes more sense for terrain details. Instead of deleting the lightmap sampling code, this makes it more generic so both lightmap or SH can be used.
@KEngelstoft
Copy link
Contributor Author

That should be fixed b

Fixed here 95e5f15

Copy link
Contributor

@Verasl Verasl left a comment

Choose a reason for hiding this comment

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

LGTM

@ernestasKupciunas ernestasKupciunas requested a review from a team April 13, 2021 09:22
@aleksandrasdzikia aleksandrasdzikia requested review from aleksandrasdzikia and removed request for a team April 13, 2021 09:39
Copy link

@aleksandrasdzikia aleksandrasdzikia left a comment

Choose a reason for hiding this comment

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

Tested visual correctness, results are good. Tested URP Examples and Boat Attack projects. Found some bugs, but they are not related to this PR and occurs on Master as well, will report them if not reported yet. Otherwise, looks good on mobile side.

Copy link
Contributor

@hdb-unity hdb-unity left a comment

Choose a reason for hiding this comment

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

Tested on commit 95e5f15.

Test doc linked on Jira.
27 issues found, no regressions from PR found.

@phi-lira phi-lira merged commit 3c06f6d into master Apr 15, 2021
@phi-lira phi-lira deleted the universal/enlighten-support branch April 15, 2021 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants