Fixing color input to gamma correct

[elizabeth-legros]

Needed a way to deprecate the Color Property and Color Node as painlessly as possible. Made system to handle deprecation/upgrading, as well as explicit override to allow previous version usage.

Color property deprecation - exposed HDR color properties are passed through without modification to shader. Need to be corrected. Color picker assumes linear space, so assume HDR color properties are in linear space, and conditionally convert to gamma based on project.
Color node deprecation - Inline colors were always assumed to be in gamma space, and conditionally were converted to linear space. HDR colors are already in linear space, so fixed conditional space conversion.

Modified input nodes test in test project to test (HDR and Default) inline color node, regular color property, and deprecated color property

[elizabeth-legros]

This should be static and override-able, but I am having a brain fart on the way to do that

[marctem]

A few comments (any questions I didn't answer I don't know =) ).

The "Enable Depredation" checkbox also needs to be added to our docs on a "Preferences" page (I think one exists but it should if it doesn't =) ).

[sharlenet]

My suggestion is to move this note higher up (before Default) to make it clearer that this note applies to all color properties. Also, here's my re-write. I'm waiting for Liz to confirm that the info is correct.

NOTE: Previous versions actually interpreted the color in linear space. The behavior is corrected now, and previously created color properties will continue to use old behavior unless explicilty upgraded through the [Graph Inspector](Internal-Inspector.md). You may use a **Vector 4** property or convert a new **Color** property back to linear space with the [Colorspace Conversion Node](Colorspace-Conversion-Node.md) to mimic old behavior.

[ghost]

Noted in the document but I want to mention here too, I think that the upgrade button should be there regardless of whether "enable deprecated nodes" is enabled. Having that enabled doesn't necessarily mean that the user doesn't want to upgrade nodes, especially in cases where they want to be able to create deprecated versions for some nodes but want to upgrade other nodes.

[ghost]

I'm finding that there is not enough warning to the user that upgrading the deprecated node will change the resulting color/emission level. Users might not mouse over the warning and there are cases that you can not always see a change in the shadergraph master preview, so the user might upgrade, then do a bunch of additional work (so they can't undo), then go to the scene and see that the emission level has changed. I feel like we need to rethink how we do this warning, perhaps have a popup explaining that this will change the resulting color/emission if it's HDR, and the user may need to fine tune the color again to get the old result. We may want to make such pop up standard for any upgrade that causes a noticeable change in functionality.

[marctem]

I'm finding that there is not enough warning to the user that upgrading the deprecated node will change the resulting color/emission level.

We don't want users to be confused but I think the tooltip on the node, the message in the inspector, and info in the upgrade guide is sufficient. No matter what we do someone will miss it. I'm OK with it as-is.

[sharlenet]

We don't want users to be confused but I think the tooltip on the node, the message in the inspector, and info in the upgrade guide is sufficient. No matter what we do someone will miss it. I'm OK with it as-is.

I'd like to edit the documentation (have reached out to Liz about this), and wouldn't mind looking over the tooltip/Inspector message too.

[ghost]

My last concern of being able to update the node while deprecated nodes are enabled in preferences, was addressed.

[cdxntchou]

Hmm... did a quick test and I can't open any shadergraphs.

Exception thrown while invoking [OnOpenAssetAttribute] method 'UnityEditor.ShaderGraph.ShaderGraphImporterEditor:OnOpenAsset (int,int)' : NullReferenceException: Object reference not set to an instance of an object
UnityEditor.ShaderGraph.BlockNode.get_index () (at D:/TestProjects/GraphicsVT/com.unity.shadergraph/Editor/Data/Nodes/BlockNode.cs:46)
UnityEditor.ShaderGraph.Drawing.GraphEditorView+<>c.b__88_0 (UnityEditor.ShaderGraph.BlockNode s) (at D:/TestProjects/GraphicsVT/com.unity.shadergraph/Editor/Drawing/Views/GraphEditorView.cs:995)
System.Linq.EnumerableSorter2[TElement,TKey].ComputeKeys (TElement[] elements, System.Int32 count) (at <351e49e2a5bf4fd6beabb458ce2255f3>:0) System.Linq.EnumerableSorter1[TElement].ComputeMap (TElement[] elements, System.Int32 count) (at <351e49e2a5bf4fd6beabb458ce2255f3>:0)
System.Linq.EnumerableSorter1[TElement].Sort (TElement[] elements, System.Int32 count) (at <351e49e2a5bf4fd6beabb458ce2255f3>:0) System.Linq.OrderedEnumerable1[TElement].SortedMap (System.Linq.Buffer1[TElement] buffer) (at <351e49e2a5bf4fd6beabb458ce2255f3>:0) System.Linq.OrderedEnumerable1+d__3[TElement].MoveNext () (at <351e49e2a5bf4fd6beabb458ce2255f3>:0)
UnityEditor.ShaderGraph.Drawing.GraphEditorView.AddBlocks (System.Collections.Generic.IEnumerable`1[T] blocks) (at D:/TestProjects/GraphicsVT/com.unity.shadergraph/Editor/Drawing/Views/GraphEditorView.cs:995)
UnityEditor.ShaderGraph.Drawing.GraphEditorView..ctor (UnityEditor.EditorWindow editorWindow, UnityEditor.ShaderGraph.GraphData graph, UnityEditor.Graphing.Util.MessageManager messageManager) (at D:/TestProjects/GraphicsVT/com.unity.shadergraph/Editor/Drawing/Views/GraphEditorView.cs:264)
UnityEditor.ShaderGraph.Drawing.MaterialGraphEditWindow.Initialize (System.String assetGuid) (at D:/TestProjects/GraphicsVT/com.unity.shadergraph/Editor/Drawing/MaterialGraphEditWindow.cs:890)
UnityEditor.ShaderGraph.ShaderGraphImporterEditor.ShowGraphEditWindow (System.String path) (at D:/TestProjects/GraphicsVT/com.unity.shadergraph/Editor/Importers/ShaderGraphImporterEditor.cs:123)
UnityEditor.ShaderGraph.ShaderGraphImporterEditor.OnOpenAsset (System.Int32 instanceID, System.Int32 line) (at D:/TestProjects/GraphicsVT/com.unity.shadergraph/Editor/Importers/ShaderGraphImporterEditor.cs:132)
UnityEngine.GUIUtility:ProcessEvent(Int32, IntPtr, Boolean&) (at C:/ono/trunk2/Modules/IMGUI/GUIUtility.cs:189)

[cdxntchou]

(but it works in the master I merged in) ..

[sharlenet]

Everything looks good! If you backport to 9.0, need to make sure that all instances of "10.0" are changed to "9.0".

[cdxntchou]

Hmm... did a quick test and I can't open any shadergraphs.

Fixed in latest!