Skip to content

Conversation

@nerites
Copy link
Contributor

@nerites nerites commented Mar 17, 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 a graphics test in HDRP for SpeedTree8. Also added SpeedTrees to the terrainlit and terrainbaselit test scenes.
Added the SpeedTree8 shadergraph to URP and replacing existing shader as default.
Replaced the SpeedTree8 shaders used in universal tests that involve ST8 assets.

TODO: Move SpeedTree8MaterialUpgrader from Core to Shadergraph package? -> Even invoking MaterialUpgrader using Core.Editor.MaterialUpgrader does not find the class from the shadergraph package, for some reason. Why?

21.1 backport: #4068
10.x.x backport: #3861


Testing status

Added a SpeedTree8 test to HDRP and added ST8 assets to the TerrainLit + Basemap tests in HDRP.

Tested importing new ST8 assets in both Universal and HD.
Also tested upgrading, but that was many changes ago and is worth re-testing. To do this, must comment out the OnPostprocessSpeedTreeAsset callback in the MaterialPostprocessor in either pipeline, then add a ST8 asset to the project. Then, running the upgrader should preserve properties such as wind quality, billboard, etc.

Tested Wind, Subsurface, and billboard work correctly on both.

Need to test huevariation.

Backface culling is not working on the Universal shadergraph- I need to set up some way of setting that per-material in Universal. I'm thinking of just copying what the HD Lit target does.

Universal Shadergraph tests look significantly different in Yamato runs of tests, even though running locally passes just fine. This needs investigation.


Comments to reviewers

@github-actions
Copy link

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page)
and cancel any jobs that started on Yamato.
See the PR template for more information.
Thank you!

@nerites
Copy link
Contributor Author

nerites commented Mar 17, 2021

Backport is here: #3861

@nerites nerites marked this pull request as draft March 18, 2021 22:44
@nerites nerites changed the title Add SpeedTree8 shadergraph as default ST8 shader in HD and Universal Add SpeedTree8 shadergraph to HD and Universal Mar 24, 2021
@JMargevics JMargevics self-requested a review March 26, 2021 07:35
@nerites nerites marked this pull request as ready for review April 1, 2021 06:32
@nerites
Copy link
Contributor Author

nerites commented Apr 1, 2021

Backport to 2021.1: #4068

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I think I've tested this enough, it should be good now.

@nerites
Copy link
Contributor Author

nerites commented Apr 5, 2021

The remaining Yamato failures are all hybrid tests that timeout due to either a firewall or nvidia desktop popup, or pipelines that have a dependency on a failing hybrid test. These failures occur in master as well.

Copy link

@xiaoxicici xiaoxicici left a comment

Choose a reason for hiding this comment

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

Tested the latest in HDRP, notes can be found in this doc: https://docs.google.com/document/d/1vue-NcZoeNGGHMlLTRiqWY-QcXy0Hx4Tv6IWXn4ioZw/edit#

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.

Test doc

❕ One major issue left: no trees visible in Metal build. @nerites is aware of this issue, it will take time to get fixed. We don't want to stall this PR any further, so I'm approving this as is. Will report this to FogBugz for better tracking.

We will be working to add ST8+terrains coverage into runtime tests 👍

✅ Otherwise there are no major issue on all other platforms. PR has been extensively tested, including consoles. ST8 shader and importer work nicely with wind physics and HDRP systems.

@nerites
Copy link
Contributor Author

nerites commented Apr 20, 2021

The remaining failure in URPUpdate_Top_Asset_Store is fixed in this PR and not due to my changes: https://github.cds.internal.unity3d.com/sophia/URP-Update-testing/pull/1

@lumonix
Copy link

lumonix commented Apr 20, 2021

Grant and I have discussed that we will allow this PR to merge even though URP QA has not been able to finish their testing on this, because we do not want to miss the 10.5 deadline.
We've done URP testing on our end and will follow up promptly with any bug fixes in case URP QA finds any.

@GrantLamb-Unity
Copy link
Contributor

Update after further discussions - Since it looks like the deadline is by EoD tomorrow ( 4/21 ) I'd like for us to not merge and I'll chat with URP QA. They should be the ones who would allow a merge at this point or not.

Additionally Landon is out today and I'd like to ask him some follow up questions about his tests, which we could potentially share with URP and see if it's possible that we have sufficient coverage via the tests that have already been done.

@nerites nerites merged commit a546058 into master Apr 21, 2021
@nerites nerites deleted the speedtreesg branch April 21, 2021 16:40
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.

10 participants