RP Workflows - Adding the capability to revert a single override of a Volume Component by alex-vazquez · Pull Request #6129 · Unity-Technologies/Graphics · GitHub
Skip to content

RP Workflows - Adding the capability to revert a single override of a Volume Component#6129

Merged
alex-vazquez merged 8 commits into
masterfrom
rpw/revert-single-volume-parameter
Nov 25, 2021
Merged

RP Workflows - Adding the capability to revert a single override of a Volume Component#6129
alex-vazquez merged 8 commits into
masterfrom
rpw/revert-single-volume-parameter

Conversation

@alex-vazquez

Copy link
Copy Markdown
Contributor

Purpose of this PR

JIRA: https://jira.unity3d.com/browse/XPIPELINE-343

Adding a context menu "Revert Property Override" for Property Fields of Volume Components

image


Testing status

Check that the restore of the values works fine.
Check that the override is NOT restored, this should only be applied when Resetting the full Volume Component.


Comments to reviewers

I've used the system from IApplyRevertPropertyContextMenuItemProvider, unfortunatelly we could not replace the name of the context menu as this is generated from the "Revert + GetSourceTerm() + override". As this system is already working, and well tested, I think counterproductive to implement a new system just to remove the override, in this scenario makes sense and we won't need to touch trunk code for this PR.

@alex-vazquez alex-vazquez requested review from a team and arttu-peltonen and removed request for a team October 25, 2021 14:14
@alex-vazquez alex-vazquez self-assigned this Oct 25, 2021
@github-actions

github-actions Bot commented Oct 25, 2021

Copy link
Copy Markdown

var o = parameter.GetObjectRef<MinFloatParameter>();
float v = EditorGUILayout.FloatField(title, value.floatValue);
value.floatValue = Mathf.Max(v, o.min);
EditorGUILayout.PropertyField(value, title);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Float fields do not implement the context menu, just property fields.

/// </example>
[Serializable]
#if UNITY_EDITOR
public class VolumeComponent : ScriptableObject, IApplyRevertPropertyContextMenuItemProvider

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think is nasty, but the interface is defined on editor, only for MonoBehaviours, I've not seen any other place where is being used, if you know a better way of doing this

@arttu-peltonen

Copy link
Copy Markdown
Contributor

Hey, could you check if this works with prefab instances & variants? I have some concerns because I checked how the the interface is used and it's invoked with this condition:

// Only display the custom apply/revert menu for GameObjects/Components that are not part of a Prefab instance & variant.
var shouldDisplayApplyRevertProviderContextMenuItems = targetObject is IApplyRevertPropertyContextMenuItemProvider
   && PrefabUtility.GetPrefabInstanceHandle(targetObject) == null;

It's good to reuse existing code but if this interface doesn't work in all cases, you might need to modify trunk after all...

@alex-vazquez

Copy link
Copy Markdown
Contributor Author

@arttu-peltonen I've checked the Prefab, and the instances, it works as expected.

@alex-vazquez alex-vazquez requested a review from a team October 29, 2021 10:40
@alex-vazquez alex-vazquez marked this pull request as ready for review October 29, 2021 10:40

@arttu-peltonen arttu-peltonen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In that case, it should be fine. Tagging @RSlysz to be aware.

@JMargevics

Copy link
Copy Markdown
Contributor

Hey, found a some generic volume component overrides and some specific ones that don't have a revert function.

image
All color pickers. I think it is very important to bring the revert feature to color pickers as well!

image
All texture pickers.

image
Cloud Layer. Anything in Layer A section.

image
Gradient, HDRI, PBR skies. All intensity modes.

And the final issue is that any Revert action is not undoable.: if you revert a value you stick to it and there's no way to return to previous one.

@alex-vazquez

Copy link
Copy Markdown
Contributor Author

@JMargevics I think all parameters can be reset now :)

@JMargevics

JMargevics commented Nov 22, 2021

Copy link
Copy Markdown
Contributor

@JMargevics JMargevics left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All perfect now 👍✔

@alex-vazquez alex-vazquez merged commit d6f32ad into master Nov 25, 2021
@alex-vazquez alex-vazquez deleted the rpw/revert-single-volume-parameter branch November 25, 2021 09:33
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.

6 participants