Skip to content

Conversation

@nerites
Copy link
Contributor

@nerites nerites commented Mar 6, 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.
Added a graphics test in HDRP for SpeedTree8. Also added SpeedTrees to the terrainlit and terrainbaselit test scenes.

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?


Testing status

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

@LandonTownsendUnity do you mind checking if the normal flipping is behaving as you'd expect in HDRP?
@adrien-de-tocqueville for changes to HDShaderUtils.ResetMaterialKeywords.
@cdxntchou for placement of files within shadergraph and core packages. Notably, LODCrossfade is not specific to SpeedTree. Maybe it should be placed someplace else/exposed generally somehow?
@phi-lira for feedback on adding the shadergraph to Universal and not hooking it up since it's not intended to replace the default yet.

@sebastienlagarde sebastienlagarde requested a review from a team March 10, 2021 09:20
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.

the material keyword reset code lgtm

@kroatoa
Copy link
Contributor

kroatoa commented Mar 12, 2021

Am I right in assuming that this PR effectively targets 19.4?
I don't see any mention of this having landed in branches targeting the later versions, how is that tracked?

@nerites
Copy link
Contributor Author

nerites commented Mar 12, 2021

@kroatoa Since shadergraph is not backwards compatible, I started with 19.4 and am making PRs for later versions as I get them ready. I will have a 20.2 PR up later today, but it's not tracked anywhere outside my worklog for now. :)

@nerites nerites requested review from cdxntchou and phi-lira March 12, 2021 22:18
@nerites nerites changed the title Add SpeedTree8 shadergraph to HDRP and URP [7.x.x] Add SpeedTree8 shadergraph to HDRP and URP Mar 12, 2021
@nerites
Copy link
Contributor Author

nerites commented Mar 12, 2021

10.x.x version here. No changes from 7.x.x. #3861

Copy link
Contributor

@kroatoa kroatoa 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 clarifying

@nerites
Copy link
Contributor Author

nerites commented Mar 16, 2021

@kroatoa I have some API changes that will require incrementing package versions for Core and HDRP. How do I decide whether to increment minor or major versions? Who do I need to get approval from for incrementing the versions of those packages?

@phi-lira
Copy link
Contributor

Did the PR to master landed? I can only find this one but it was closed: #3713

@nerites
Copy link
Contributor Author

nerites commented Mar 17, 2021

@phi-lira No, I am remaking it. I can have a new PR to master up today. I started with 19.4 since Shadergraph isn't backwards compatible, so it was easier to work from 19.4 and go forward.

universal ST8 shader. Convert backface normal enable to enum.
now mirror. Double sided is disabled for billboard.
@nerites nerites force-pushed the 7.x.x/speedtreesg branch from fa1d428 to 2071a30 Compare March 17, 2021 21:54
@kroatoa
Copy link
Contributor

kroatoa commented Mar 18, 2021

@kroatoa I have some API changes that will require incrementing package versions for Core and HDRP. How do I decide whether to increment minor or major versions? Who do I need to get approval from for incrementing the versions of those packages?

Please reach out to the RM for the branch you are targeting. if it's more than one you can also put a post in #release-management, and we'll take it from there.

@nerites
Copy link
Contributor Author

nerites commented Mar 18, 2021

After some discussion with other folks, I'm only targeting 10.x.x and latest. PRs here:
10.x.x: #3861
Latest: #3911

@nerites nerites closed this Mar 18, 2021
@sebastienlagarde sebastienlagarde deleted the 7.x.x/speedtreesg branch September 1, 2021 10:18
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.

6 participants