XPipeline- Fix contextual menu Remove Component for lights and cameras #5431

alex-vazquez · 2021-08-25T10:11:15Z

Purpose of this PR

Fix https://fogbugz.unity3d.com/f/cases/1358990/

The Light Additional Data and Camera Additional Data were not being removed properly.

Testing status

Test that all additional datas are being removed when having both pipelines installed
Test that additional data remove is blocked, and they can only be removed when the component where they are additional is being removed.

Comments to reviewers

Adding the attribute to know that a component is additional data for another component.

github-actions · 2021-08-25T10:11:29Z

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://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, 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
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

URP
/.yamato%252Fall-urp.yml%2523PR_URP_2021.2

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:
/.yamato%252F_abv.yml%2523all_project_ci_2021.2
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.

martint-unity

LGTM

arttu-peltonen

Looks great! I'd like to hear Rémi's thoughts on the attribute usage though. Also please check if the interface I mentioned could be removed/obsoleted.

arttu-peltonen · 2021-08-26T07:59:01Z

com.unity.render-pipelines.core/Runtime/Common/CoreAttributes.cs

+
+    /// <summary>
+    /// Attribute to specify that a class is additional data of another component
+    /// Currently is being used for HDRP/URP additional Camera and Light data
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Class)]
+    public class AdditionalComponentData : Attribute
+    {
+        /// <summary>
+        /// Summary the component type of is additional data
+        /// </summary>
+        public Type componentType;
+
+        /// <summary>
+        /// Constructor of the attribute
+        /// </summary>
+        /// <param name="componentType">The component type</param>
+        public AdditionalComponentData(Type componentType)
+        {
+            this.componentType = componentType;
+        }
+    }


I'm interested to hear what @RSlysz you think about this, in light of possible future work on additionaldata refactor. Currently AFAIK there's no IAdditionalComponentData interface or similar, so this attribute now allows us to detect these components in SRP Core code. If there's already a plan to introduce this interface, then this attribute might be redundant. But even if so, it probably doesn't really hurt to add the attribute. So I'm fine with this, just wanted to raise this point :)

We didn't really need them as we previously considered that this was an editor workflow to manage. So we ended with having interface but on Editor side (such IRemoveAdditionalDataContextualMenu<Camera>).

And the plan was to remove the additional data sibling component. So when we will be able to finish & land this, the attribute introduced here (or the former interface) will don't have any usage. That is why even if the need for it increased last year, I didn't add it as it add a public API that should be deprecated soon. :)

Now as the additional data refactor is just so overwhelming and it is a nightmare to know exactly when this will be finished, I'm ok with this introduction. But as it is public API, some user can want to use it in their custom SRP with component that we have not think of yet. Should we ensure that if this happens, can we ensure our system will works in this case?

Also, HD have additional component for Reflection Probe too (regarding comments). There is no conflict as URP don't have additional data for this so there is also no contextual menu dispatcher. But I would prefer to unify our workflow and thus use this component also for HDReflectionProbe.

Also as we partly automate the concept of additional data here, as a user, I would expect to not have to write RemoveAdditionalComponent contextual menu command and such when I wrote my own SRP. But this seams tricky to automate...

Lastly, I wonder if this should inherite [RequireComponent(typeof(Camera))] as it seams to me we always have this one when using the [AdditionalComponentData(typeof(Camera))].

I will have a look at the Reflection Probe and how to automate the context menu.

The RequierComponent is sealed, so we can not inherit from it.

Since we also discussed inheriting from RequireComponent, I wonder if we could make it non-sealed? I'm not sure if there's a good reason to keep it sealed.

com.unity.render-pipelines.core/Editor/Debugging/DebugState.cs

arttu-peltonen · 2021-08-26T10:23:09Z

com.unity.render-pipelines.core/Editor/ContextualMenuDispatcher.cs

            {
-                IRemoveAdditionalDataContextualMenu<T> instance = (IRemoveAdditionalDataContextualMenu<T>)Activator.CreateInstance(type);
-                instance.RemoveComponent(component, ComponentDependencies(component));
+                IRemoveAdditionalDataContextualMenu<T> instance = new RemoveAdditionalDataContextualMenu<T>();


Is IRemoveAdditionalDataContextualMenu still needed for something? AFAICT it only has one implementation, which is now also in SRP Core, so it seems redundant. Seems like it should be marked obsolete as it no longer does anything for you even if you write a custom context menu that inherits from it (since FetchFirstCompatibleTypeUsingScriptableRenderPipelineExtension<IRemoveAdditionalDataContextualMenu<T>>() was removed).

I will mark it as obsolete :)

RSlysz

LGTM

Though I wonder if we can do better when introducing this attribute (see comments)

RSlysz · 2021-08-30T12:36:04Z

com.unity.render-pipelines.core/Runtime/Common/CoreAttributes.cs

+
+    /// <summary>
+    /// Attribute to specify that a class is additional data of another component
+    /// Currently is being used for HDRP/URP additional Camera and Light data
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Class)]
+    public class AdditionalComponentData : Attribute
+    {
+        /// <summary>
+        /// Summary the component type of is additional data
+        /// </summary>
+        public Type componentType;
+
+        /// <summary>
+        /// Constructor of the attribute
+        /// </summary>
+        /// <param name="componentType">The component type</param>
+        public AdditionalComponentData(Type componentType)
+        {
+            this.componentType = componentType;
+        }
+    }


We didn't really need them as we previously considered that this was an editor workflow to manage. So we ended with having interface but on Editor side (such IRemoveAdditionalDataContextualMenu<Camera>).

And the plan was to remove the additional data sibling component. So when we will be able to finish & land this, the attribute introduced here (or the former interface) will don't have any usage. That is why even if the need for it increased last year, I didn't add it as it add a public API that should be deprecated soon. :)

Now as the additional data refactor is just so overwhelming and it is a nightmare to know exactly when this will be finished, I'm ok with this introduction. But as it is public API, some user can want to use it in their custom SRP with component that we have not think of yet. Should we ensure that if this happens, can we ensure our system will works in this case?

Also, HD have additional component for Reflection Probe too (regarding comments). There is no conflict as URP don't have additional data for this so there is also no contextual menu dispatcher. But I would prefer to unify our workflow and thus use this component also for HDReflectionProbe.

Also as we partly automate the concept of additional data here, as a user, I would expect to not have to write RemoveAdditionalComponent contextual menu command and such when I wrote my own SRP. But this seams tricky to automate...

Lastly, I wonder if this should inherite [RequireComponent(typeof(Camera))] as it seams to me we always have this one when using the [AdditionalComponentData(typeof(Camera))].

RSlysz · 2021-08-30T12:40:54Z

...ty.render-pipelines.high-definition/Editor/RenderPipeline/Camera/HDAdditionalCameraEditor.cs

        }
+
+        [MenuItem("CONTEXT/HDAdditionalCameraData/Remove Component")]
+        static void RemoveComponent(MenuCommand command)


Can we unify names? On light it is RemoveAdditionalComponent and on Camera it is RemoveComponent

RSlysz · 2021-08-30T12:44:01Z

...ity.render-pipelines.high-definition/Editor/RenderPipeline/Camera/HDCameraEditor.Handlers.cs

                return;

-            var c = (Camera)target;
+            if (!(target is Camera c) || c == null)


I'm curious: target is Camera c if target is null, c is (Camera)null right? Then why do we need the null check?

because is null on the c++ world, not in c# the is operator is not overriden

alex-vazquez · 2021-08-31T11:52:46Z

@RSlysz, updates on the PR:

Generic method to add all the Remove Component methods for additional datas
Unified HDReflectionProbe

@arttu-peltonen , the RequiereComponent not being sealed, maybe I will tackle it in another PR.

alex-vazquez · 2021-08-31T13:45:20Z

@Unity-Technologies/gfx-qa-hdrp

Test camera, lights, and reflection probes, delete them and see that the additional data is removed.
Try to see that the additional data is blocked

@Unity-Technologies/gfx-qa-urp

Test camera and lights, delete them and see that the additional data is removed.
Try to see that the additional data is blocked

fredericv-unity3d

I have only one concern that is not directly related to your PR:

CoreUtils has a whole section of UNITY_EDITOR functions, which is asking for trouble in my opinion.

This definitely needs a refactoring to move editor code into editor assemblies

TomasKiniulis

HDRP side all good. Checked with Lights, Cameras, Probes with HDRP and URP additional data assigned.

Deleting additional data component is correctly blocked
Deleting main component also clears additional data, no errors in console
Undo/Redo correctly recreates deleted components

erikabar

developer already tested this PR, approving without additional coverage

#5431

sebastienlagarde · 2021-09-12T18:43:56Z

@alex-vazquez : I reverted the backport I have done due to this error on 21.2

18:31:52.686 Information] Compiler
severity: Error
message: C:\build\output\Unity-Technologies\Graphics\com.unity.render-pipelines.core\Editor\ContextualMenuDispatcher.cs(70,21): error CS0103: The name 'MenuManager' does not exist in the current context
stacktrace:
line: 7

…d cameras #5431

alex-vazquez · 2021-09-13T07:03:38Z

@sebastienlagarde : Sorry Seb, this PR needs to be merged before, I forgot the backport label :( #5547

…#5431

alex-vazquez requested review from RSlysz, arttu-peltonen and martint-unity August 25, 2021 10:11

alex-vazquez self-assigned this Aug 25, 2021

alex-vazquez changed the title ~~Universal/fix contextual menus for light camera~~ XPipeline- Fix contextual menu Remove Component for lights and cameras Aug 25, 2021

martint-unity approved these changes Aug 25, 2021

View reviewed changes

arttu-peltonen reviewed Aug 26, 2021

View reviewed changes

RSlysz approved these changes Aug 30, 2021

View reviewed changes

alex-vazquez requested review from RSlysz and arttu-peltonen August 31, 2021 11:51

RSlysz approved these changes Aug 31, 2021

View reviewed changes

arttu-peltonen approved these changes Aug 31, 2021

View reviewed changes

alex-vazquez requested review from a team August 31, 2021 13:45

alex-vazquez marked this pull request as ready for review August 31, 2021 13:45

alex-vazquez requested a review from a team as a code owner August 31, 2021 13:45

github-actions bot added HDRP SRP universal labels Aug 31, 2021

alex-vazquez added the needs-backport-2021.2 label Sep 1, 2021

alex-vazquez requested a review from fredericv-unity3d September 1, 2021 09:40

fredericv-unity3d approved these changes Sep 1, 2021

View reviewed changes

alex-vazquez mentioned this pull request Sep 7, 2021

SRP Workflows - Menu Manager #5547

Merged

alex-vazquez-unity3d added 2 commits September 8, 2021 13:14

SRP Workflows - Fix additional data's components remove

72a0f08

Remove usings

acf4713

alex-vazquez force-pushed the universal/fix-contextual-menus-for-light-camera branch from 0484780 to acf4713 Compare September 8, 2021 11:17

Fix crash with Reflection Probe

915c70e

TomasKiniulis approved these changes Sep 8, 2021

View reviewed changes

alex-vazquez requested review from martint-unity and removed request for a team September 9, 2021 06:54

martint-unity approved these changes Sep 9, 2021

View reviewed changes

Format

f868104

alex-vazquez requested a review from a team as a code owner September 9, 2021 07:14

erikabar approved these changes Sep 9, 2021

View reviewed changes

alex-vazquez merged commit cf2d4eb into master Sep 9, 2021

alex-vazquez deleted the universal/fix-contextual-menus-for-light-camera branch September 9, 2021 08:32

sebastienlagarde added backported-2021.2 and removed needs-backport-2021.2 labels Sep 12, 2021

sebastienlagarde pushed a commit that referenced this pull request Sep 12, 2021

XPipeline- Fix contextual menu Remove Component for lights and cameras

5a914fd

#5431

sebastienlagarde added needs-backport-2021.2 and removed backported-2021.2 labels Sep 12, 2021

sebastienlagarde added a commit that referenced this pull request Sep 12, 2021

Revert: XPipeline- Fix contextual menu Remove Component for lights an…

b7ff05d

…d cameras #5431

sebastienlagarde added backported-2021.2 and removed needs-backport-2021.2 labels Sep 20, 2021

sebastienlagarde pushed a commit that referenced this pull request Sep 20, 2021

XPipeline- Fix contextual menu Remove Component for lights and cameras …

c70cb90

…#5431

XPipeline- Fix contextual menu Remove Component for lights and cameras #5431

XPipeline- Fix contextual menu Remove Component for lights and cameras #5431

Uh oh!

Conversation

alex-vazquez commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

github-actions bot commented Aug 25, 2021

Uh oh!

martint-unity left a comment

Choose a reason for hiding this comment

Uh oh!

arttu-peltonen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RSlysz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-vazquez commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-vazquez commented Aug 31, 2021

Uh oh!

fredericv-unity3d left a comment

Choose a reason for hiding this comment

Uh oh!

TomasKiniulis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikabar left a comment

Choose a reason for hiding this comment

Uh oh!

sebastienlagarde commented Sep 12, 2021

Uh oh!

alex-vazquez commented Sep 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

alex-vazquez commented Aug 25, 2021 •

edited

Loading

alex-vazquez commented Aug 31, 2021 •

edited

Loading

TomasKiniulis left a comment •

edited

Loading