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

Make shader keyword check compatible with Shader Graph shader #781

Merged
merged 2 commits into from
Mar 14, 2021

Conversation

daleeidd
Copy link
Collaborator

@daleeidd daleeidd commented Mar 5, 2021

Shader keywords differ with SG. They either have a prefix or are missing. The point of this PR is to reduce diff noise and increase compatibility. Hopefully, this will be a temporary solution.

Adds a MATERIAL_KEYWORD_PREFIX constant which is used:

internal const string MATERIAL_KEYWORD = MATERIAL_KEYWORD_PREFIX + "_FOAM_ON";

For built-in, the prefix is an empty string, but CREST_ will be added in HDRP (diff noise).

MATERIAL_KEYWORD_PROPERTY

The next is skipping any keyword where the OceanMaterial.HasProperty is false. This checks the property name. So for foam we check "_Foam" which matches with "FOAM_ON".

So with SG, if we do not have a keyword defined, then it will be skipped.

MATERIAL_KEYWORD_PROPERTY has been added which will need to be changed in HDRP (diff noise).

This will give us time to consider #769 more carefully. If that PR is merged only the prefix change needs to be reverted. The HasProperty fix will probably need to stay.

@daleeidd daleeidd requested review from huwb and removed request for huwb March 5, 2021 10:55
@daleeidd daleeidd marked this pull request as draft March 5, 2021 11:01
@daleeidd daleeidd marked this pull request as ready for review March 5, 2021 11:05
@daleeidd daleeidd requested a review from huwb March 5, 2021 11:05
Copy link
Contributor

@huwb huwb left a comment

Choose a reason for hiding this comment

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

I didn't quite get why the has property but it's required? But feel free to merge in the meantime

@daleeidd
Copy link
Collaborator Author

daleeidd commented Mar 5, 2021

It's for when we don't have a keyword in HDRP like for the clip surface. Currently, the checks will trigger the validation messages (unless we disable the code). Unfortunately, there is no "HasKeyword". So we check if the keywords property counterpart exists instead. I will do a few more tests before merging so it is robust. I think there might be a better solution.

@huwb
Copy link
Contributor

huwb commented Mar 6, 2021

Oh gotcha

@daleeidd
Copy link
Collaborator Author

After checking it seems robust. I'll push this through to get the keyword check working downstream.

@daleeidd daleeidd merged commit f1a4004 into master Mar 14, 2021
@daleeidd daleeidd deleted the misc/shader-keyword-check-compat branch April 1, 2021 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants