Skip to content

Conversation

@nerites
Copy link
Contributor

@nerites nerites commented Mar 12, 2021

Please read the Contributing guide before making a PR.

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

Adding SpeedTree8 support (including material upgrader) to HDRP.
Added the SpeedTree8 shadergraph to URP as well, though the default is still the original SpeedTree8 shader pending wind support in the shadergraph.
No changes from the 7.x.x version of this PR, aside from what was necessary to merge with 10.x.x version of HDRP.

21.1 PR: #4068
PR to master: #3911


Testing status

(Copied from 7.x.x)
Tested importing and material upgrading in HDRP. It works, except for an issue with embedded materials where when the Library is nuked, the materials don't get setup properly. This is because the speedtree asset is imported before the HDRP asset is imported, so the correct shader is not grabbed, and the correct postprocessing is not run. This is a recurring issue and topic of discussion not unique to SpeedTree, so it should not be blocking for this PR.

For URP, tested that manually switching over the shader works as expected. No upgrader or importer support for the shadergraph is needed yet as it is not yet the default shader.


Comments to reviewers

@adrien-de-tocqueville since you signed off on the 7.x.x version of this, and the 10.x.x is unchanged.
@LandonTownsendUnity for a QA pass on HDRP (should behave the same as 7.x.x).

Shader graph: Please confirm placement of new subshadergraphs I've added to the shadergraph package, including changelog notes. I also added a LODCrossfade hlsl file. This is not ST8-specific; is there a better way of adding this?

@adrien-de-tocqueville
Copy link
Contributor

Sorry about the missclick ! Marked the PR as ready by mistake, but it looks like I cannot remove the reviewers that were automatically added

Copy link
Contributor

@adrien-de-tocqueville adrien-de-tocqueville left a comment

Choose a reason for hiding this comment

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

Looks the same as the PR for 7.x.x

LODCrossfade explaining its redundancy with existing Common.hlsl code.
@ValGrimm-U3D
Copy link
Contributor

@ValGrimm-U3D I incorporated your feedback in this changeset:
b15bf84
Let me know what you think :) Thanks!

Thank you (:
I've looked through your responses and added my own, resolving the conversation where relevant.

Additional changes here: b22e9fc

Looks great! Thank you.

(In general I would avoid passive voice, but you've already revised that sentence once, and it isn't worth nitpicking that much in a changelog. )

Copy link
Contributor

@ValGrimm-U3D ValGrimm-U3D left a comment

Choose a reason for hiding this comment

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

Thanks for responding to my many questions and suggestions, fine to merge. 👍

Copy link
Contributor

@JMargevics JMargevics left a comment

Choose a reason for hiding this comment

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

https://confluence.unity3d.com/pages/viewpage.action?pageId=129564990 Test doc includes coverage of this backport. No specific issues were found, looks good from HDRP side ✅

@nerites
Copy link
Contributor Author

nerites commented Apr 19, 2021

Remaining failures are all hybrid failures discussed here, and are not due to my PR:
https://unity.slack.com/archives/CFZERNTS7/p1617956110148300

@nerites nerites requested review from a team and ellioman April 20, 2021 22:12
@ellioman ellioman requested a review from phi-lira April 23, 2021 11:03
Copy link
Contributor

@erikabar erikabar left a comment

Choose a reason for hiding this comment

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

we do not have QA bandwidth right now, we will test this after feature cutoff.

@nerites
Copy link
Contributor Author

nerites commented May 19, 2021

we do not have QA bandwidth right now, we will test this after feature cutoff.

when is that?

@GrantLamb-Unity
Copy link
Contributor

@erikabar Do you have an ETA on when we could get a URP QA review?

@erikabar
Copy link
Contributor

status update: I will finish testing by tomorrow noon

@erikabar erikabar self-requested a review May 27, 2021 09:30
Copy link
Contributor

@erikabar erikabar left a comment

Choose a reason for hiding this comment

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

Tested URP SpeedTree8_PBRLit shadergraph, everything works as expected in the Editor and Standalone build. Also tested Wind Zone, Terrain Tree painting, interaction with Spot, Point, Directional lights and shadows - no issues found.

@sebastienlagarde sebastienlagarde merged commit 45190a0 into 10.x.x/release Jun 3, 2021
@sebastienlagarde sebastienlagarde deleted the 10.x.x/speedtreesg branch June 3, 2021 18:08
pastasfuture pushed a commit that referenced this pull request Aug 20, 2021
* Squashed commits for 19.4's ST8 support

* Add wind enable/disable toggle to HD ST8 shader.

* Fixup URP SpeedTree8 shader. Hookup GI for both ST8 shaders.

* Update HD ST8 test scene to test various wind qualities and wind enable
toggle.

* Fix double-sided setting on billboard materials.

* Remove redundant keyword enable per review.

* Remove unused keyword from HD ST8

* Updated changelogs for SpeedTree8 addition.

* Add ST8 shadergraphs to HD and Universal, including supporting
subshadergraphs in ShaderGraph and MaterialUpgrader in core. Add a test
to HDRP for speedtree8.

* Fixup SpeedTree8 test rename

* Add reference images for linuxeditor/vulkan

* Fixup LOD0 material misassignment

* Clean up renamed HDLitGUI

* Update reference images from first 10.x.x yamato runs. (Darker shadows)

* Merge hue variation, crossfade, and seam blending into
SpeedTree8ColorAlpha subgraph. Fix Base Map naming in URP ST8
shadergraph. Fix wait framerate in HD ST8 test.

* Modify hue variation subgraph to match handwritten ST8 shader

* Add lerp hue variation behavior for backwards compatibility. Only expose
this property in universal ST8.

* Format files that need it

* Update reference images after fixing frame wait time for HD ST8 test

* Hook up GI and subsurface scattering in handwritten SpeedTree8 Universal
shader

* Update test materials to use a diffusion profile. Set thickness to 100
when subsurface is disabled.

* Grab default foliage diffusion profile when importing or upgrading an HD ST8
shader

* Remove subsurface enable from ST8 shader. Update ST8 test scene to use
foliage diffusion profile.

* Remove wait time from HD ST8 test

* Modulate subsurface with _SubsurfaceIndirect property

* Fix SpeedTree8 subsurface per feedback

* Update reference images for HD ST8 test after changing to foliage diffusion profile and
disabling wind

* Update SpeedTree8 shader to sample shadowmap when attenuating direct
subsurface. Update ST8 shadergraph to match subsurface calculations.

* Update shadergraph changelog after refactoring subshadergraphs for ST8.
Move LODCrossfade.hlsl out of Nature-specific folder.

* Fix hue variation randomization to work with HD's camera relative
rendering

* Change default Subsurface color to black from white

* Fix URP shadergraph default subsurface color and texture

* Update HDRP reference images once more- trunks are a slightly different
color?

* Update failing tests after handwritten ST8 shader was updated.

* Set alpha to 0 on billboard backfaces

* Update HD ST8 shadergraph after billboard subshadergraph was modified

* Normalize view direction when calculating subsurface

* Convert floats to sliders. Add a Diffusion profile property so that it
can be grouped with subsurface properties.

* Incorporate docs feedback

* Fix HD ST8 test material diffusion profiles after diffusion profile
property change

* Docs feedback on changelogs

* Clean up redundant GGX code in SpeedTree8Passes. Add comment to
LODCrossfade explaining its redundancy with existing Common.hlsl code.

* Renamed LODCrossfade to make clear its relation to the core
LODDitheringTransition function.

* Docs changes.

* Test universal iphone failure

* Docs changes

* Only generate camera motion vectors when importing ST8 asset to SRP.

* Clean up ST8 shader warnings where possible

* Update HD ST8 ref images after adding an abs() to subsurface calculation

* Update HDRP Metal reference screenshots 1601_TerrainLit

* Revert "Update HDRP Metal reference screenshots 1601_TerrainLit"

This reverts commit 39dfb3c.

Co-authored-by: Tianliang Ning <[email protected]>
Co-authored-by: Sebastien Lagarde <[email protected]>
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.