Skip to content

Conversation

alex-vazquez
Copy link
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
Copy link

github-actions bot commented Oct 25, 2021

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/jobDefinition/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

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
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
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
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
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
Copy link
Contributor

@arttu-peltonen arttu-peltonen left a comment

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
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
Contributor Author

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

@JMargevics
Copy link
Contributor

JMargevics commented Nov 22, 2021

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

All those fields work perfectly now ✔👍

Reposting my other issue for visibility:

Lil1Bcv2oL.mp4

Quality levels and underlying parameters should be connected when we revert them.

Copy link
Contributor

@JMargevics JMargevics left a comment

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