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

Add CREST_ prefix to keywords #769

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

daleeidd
Copy link
Collaborator

Adds CREST_ prefix to keywords to align with downstream. Also adds a material upgrader (the bulk of the work) to upgrade materials for users similar to SRP packages.

Let me know what you think.

Examples

There are a couple of approaches here which we can use all or only some.

Custom Shader GUI to upgrade one material:

44

InitializeOnLoadMethod and MenuItem("Edit/Crest/Upgrade Materials"):

45

How It Works

Checks the data stored in the asset to see whether any of the old m_Float properties exist. Then it inserts the new property if needed, copies the value over, does the same for the m_ShaderKeywords property, and deletes the old property. Finally, it saves the asset.

Trying to get data from the m_Floats was tough since it is an array of pairs. Would have been nice if it could have been deserialised as a dictionary.

Possible Issues

If the user decides to bypass the upgrade, they could try to fix the material themselves by using the inspector like normal. And then if they were to run the upgrade later it would overwrite what they have since the old properties would still be there.

There is also the issue of whether we want to continue using the old names at some point for something else (if we decide to keep this code around).

I believe both issues could be solved with the following:

[HideInInspector] _CrestSerializedVersion("Serialized Version", Float) = 1

It could simplify the checking too since we would just check for the serialised version.

Testing

I have committed any upgraded materials so you can try it for yourself. I have only add the prefix to four keywords: caustics, shadows, foam and flow.

@daleeidd
Copy link
Collaborator Author

An alternative which almost worked:

[Toggle(CREST_FOAM_ON)] _Foam("Enable", Float) = 1

The toggle takes a parameter to tell it what the keyword should be. Then we need to change the one keyword to the new and old:

#pragma shader_feature_local _ _FOAM_ON CREST_FOAM_ON
#if defined(_FOAM_ON) || defined(CREST_FOAM_ON)

Problem is that the new keyword validation would need to check both keywords and since this solution doesn't clear the old keyword it will give false positive.

Furthermore, it only works for Toggle. KeywordEnum doesn't have this feature.

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.

1 participant