Skip to content

Conversation

adrien-de-tocqueville
Copy link
Contributor

@adrien-de-tocqueville adrien-de-tocqueville commented Sep 22, 2021

Purpose of this PR

This PR moves the code for material validation from GUI classes to separate classes in the Runtime assembly.

It also provides a small set of function to setup complex properties, like alpha clipping, render queue, diffusion profile, and emission settings.
We could provide more but now that the reset function is available it's not essential, it's mostly to save users from searching how are properties called in the shader and if they are duplicated.

To provide an API to set diffusion profiles, I made the DiffusionProfileSetting class public, but without any public field so the profile itself can't be edited from script.

I added a [page in the doc]https://github.com/Unity-Technologies/Graphics/blob/1058e98348ab58d6c95093c8ed88b1ff68d8b378/com.unity.render-pipelines.high-definition/Documentation~/Material-API.md) that explains how to modify materials from script and how to handle variants missing in builds.

Last thing I changed is to move the code that validate displacement settings and emission color from the drawing functions themselves to the material validation. I also cleaned up some duplicate string constant.


Comments to reviewers

The PR is quite big but most of it is not really interesting, because I had to move a lot of classes around and do namespace changes.
So I sorted the changes in different commits afterwards, i think it's gonna make it easier to review code commit by commit.

For QA, there's not really any new feature, we must just make sure that there is no regression in the material validation code.
I think that would get caught by yamato, there's no practical ways to test it manually that I can think of.


Testing status

Tested the functions in the editor.
Built a player and used the API in a standalone build.

I didn't add any automatic test.

Test 1207, 1208 and 1209 for displacement were either wrong or hacked, because dispalcement parameters were only setup when going through the UI code. As a result the image changed a bit so I updated them

@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_trunk
With changes to HDRP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@Vic-Cooper
Copy link
Contributor

This PR adds a new doc that needs to go through the docs review process. I've created a ticket for this and working google doc. I'll approve this PR for now so docs aren't a blocker for you.

Copy link
Contributor

@Vic-Cooper Vic-Cooper left a comment

Choose a reason for hiding this comment

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

Please see the comment above for further docs steps.

Copy link
Member

@alelievr alelievr left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Things I've checked on my side:

  • Adjusting emissive properties on runtime, switching between intensity modes
  • Changing displacement mode, assigning height map and setting the amplitude
  • Playing with other properties

No validation errors, all good 👍✔

A few notes to add:

  • I understand that we have no shader property references for builtin and URP either. But HDRP shaders are so incredibly massive and searching for certain properties in .shader files or debug inspectors is a big pain. Some lines are left uncommented, for example _DisplacementMode("DisplacementMode", Int) = 0, and while it's obvious that 1 will be vertex mode and 2 will be pixel mode, I think it would be very advantageous to have a ref doc for our shaders.
  • @adrien-de-tocqueville added nice helper functions for setting emissive values. We have discussed that covering all shader cases would require adding many, many more, however I do believe there are certain essential ones we should add. Adrien mentioned displacement helpers and from my side I've noticed we don't have any straightforward means to interact with LayeredLit layers via API.

Copy link
Contributor

@sebastienlagarde sebastienlagarde left a comment

Choose a reason for hiding this comment

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

I forget to mention you need to update the What's new page

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.

5 participants