crest icon indicating copy to clipboard operation
crest copied to clipboard

Add CREST_ prefix to keywords

Open daleeidd opened this issue 3 years ago • 1 comments

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 avatar Feb 26 '21 07:02 daleeidd

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.

daleeidd avatar Feb 26 '21 23:02 daleeidd