[Quality Audit] Make MenuItem dynamically hiddeable for LookDev and RenderGraphWindow [OnHold] #5519

RSlysz · 2021-09-02T17:34:47Z

Purpose of this PR

MenuItem are not dynamically hideable normally. Here we added this possibility.
For LookDev and RenderGraph, we can find which RP can use them from CoreRP. So there is dedicated rules to them. This is a change from first attempt ([MenuItemForRenderPipeline]). As the attribute is no use right now, it is not in the final version of the PR. (It can still be added later though)

This is a requirement for Quality Audit in order to not have so many state (currently it can be "not here", "here but disabled", "here and enabled")

Testing status

Tested Manually:

Persistent through DomainReload (meanings the recomputation give same result)
Tested LookDev and RenderGraph both show up in HDRP and not in Universal nor Built-in

Comments to reviewers

Also adding missing namespace on the RenderGraphViewer

github-actions · 2021-09-02T17:35:00Z

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)

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_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.

alex-vazquez

Minor comments, I think we should use the Pipeline instead of the Asset though.

com.unity.render-pipelines.core/Editor/Utilities/MenuUtilities.cs

alex-vazquez · 2021-09-02T19:03:12Z

com.unity.render-pipelines.core/Editor/Utilities/MenuUtilities.cs

+            this.renderPipelineAssetTypes = renderPipelineAssetTypes
+                .Select(rpat => rpat != null ? rpat : typeof(RenderPipelineAsset)).ToArray();


I think we should throw an exception if the typeof is null or if the typeof is RenderPipelineAsset. This will mean invalid data. I think attribute must only be used to specify

This is only to support case where we want something only in builtin. Builtin is null (render pipeline asset used) but null cannot be used as a key

alex-vazquez · 2021-09-02T19:03:40Z

com.unity.render-pipelines.core/Editor/Utilities/MenuUtilities.cs

+        public readonly int priority;
+        public readonly bool validate;
+
+        public MenuItemForRenderPipeline(string path, bool validate = false, int priority = 0, params Type[] renderPipelineAssetTypes)


Are we sure we want the asset and not the pipeline?

Jenny seamed to say that the only thing we are certain at editor start is GraphicsSettings.currentRenderPipeline which is the Asset. Did I miss something @jenniferd-unity ? I would also prefer checking the RenderPipeline and not the asset if possible

alex-vazquez · 2021-09-02T19:04:50Z

com.unity.render-pipelines.core/Editor/RenderGraph/RenderGraphViewer.cs

@@ -1,690 +1,691 @@
 using UnityEngine;


Is this file from this PR?

Yes sorry I added missing name space here.
Only modification is that I remove the menu item for the RenderGraphWindow

com.unity.render-pipelines.high-definition/Editor/ToolMenus.cs

Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>

…ttps://github.com/Unity-Technologies/Graphics into srpcore/qualityaudit/menuitem-dynamically-hiddeable

RSlysz · 2021-09-02T20:36:52Z

@alex-vazquez Did you do any test for other attributes somewhere? I think I perhaps need to add some for this one. (I dunno exactly how)

jenniferd-unity

See my comments about InitializeOnLoad and not being able to know which SRP is going to be used for rendering.

jenniferd-unity · 2021-09-03T15:19:06Z

com.unity.render-pipelines.core/Editor/Utilities/MenuUtilities.cs

+
+            // Note: GraphicsSettings.currentRenderPipeline == null possible and should mean built-in
+            // though we cannot use null as key so it was early replaced by typeof(RenderPipelineAsset)
+            Type currentSRPType = GraphicsSettings.currentRenderPipeline?.GetType() ?? typeof(RenderPipelineAsset);


you can use RenderPipelineManager.currentPipeline if you want the RenderPipeline type (our APIs are very confusing I know).
The problem I can see here is that this can be called from a InitializeOnLoad where we cannot guarantee that the ADB is ready - which means we cannot guarantee accessibility to the SRP asset of the current Quality level.
So I guess your solution works in some circumstances but not all. We definitely need to push for a callback to guarantee those scenarios. (OnAssetDatabaseReady?)

We know from CoreRP if they can or cannot be used. Also cleaning code

alex-vazquez

Use the MenuManager :)

RSlysz · 2022-02-25T11:01:14Z

Update: This PR is OnHold and will remain as we still don't have a good lifecycle for render pipeline nd this is required to land it. Removing remaining reviewer.

RSlysz · 2022-04-22T11:21:13Z

Closing the PR for now

RSlysz added 4 commits September 2, 2021 18:37

Add hiddable MenuItem for SRP

5d5c01c

Update CHANGELOG.md

70b46da

Add support for built-in as null type

45cce3e

Add documentation

36e7afb

RSlysz requested a review from alex-vazquez September 2, 2021 17:34

RSlysz requested a review from JulienIgnace-Unity September 2, 2021 17:35

github-actions bot added HDRP SRP labels Sep 2, 2021

alex-vazquez suggested changes Sep 2, 2021

View reviewed changes

RSlysz and others added 5 commits September 2, 2021 22:15

Update com.unity.render-pipelines.core/Editor/Utilities/MenuUtilities.cs

2b22818

Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>

Update com.unity.render-pipelines.core/Editor/Utilities/MenuUtilities.cs

a463c74

Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>

Update com.unity.render-pipelines.core/Editor/Utilities/MenuUtilities.cs

5da8a60

Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>

Merge branch 'srpcore/qualityaudit/menuitem-dynamically-hiddeable' of h…

64fc593

…ttps://github.com/Unity-Technologies/Graphics into srpcore/qualityaudit/menuitem-dynamically-hiddeable

Add feedbacks on naming

dcbf65d

RSlysz requested a review from jenniferd-unity September 2, 2021 20:31

Fix name

538447e

RSlysz marked this pull request as ready for review September 3, 2021 13:30

jenniferd-unity suggested changes Sep 3, 2021

View reviewed changes

RSlysz added 2 commits September 7, 2021 10:38

Move LookDev and RenderGraph mechanisme into CoreRP

6657696

We know from CoreRP if they can or cannot be used. Also cleaning code

Remove Unused MenuItemForRenderPipeline

85899d3

RSlysz requested review from alex-vazquez and jenniferd-unity September 7, 2021 09:00

alex-vazquez mentioned this pull request Sep 7, 2021

SRP Workflows - Menu Manager #5547

Merged

RSlysz changed the title ~~[Quality Audit] Make MenuItem dynamically hiddeable for LookDev and RenderGraphWindow~~ [Quality Audit] Make MenuItem dynamically hiddeable for LookDev and RenderGraphWindow [OnHold] Sep 7, 2021

alex-vazquez suggested changes Sep 9, 2021

View reviewed changes

jenniferd-unity removed their request for review September 15, 2021 09:01

RSlysz removed the request for review from JulienIgnace-Unity February 25, 2022 11:00

RSlysz closed this Apr 22, 2022

		this.renderPipelineAssetTypes = renderPipelineAssetTypes
		.Select(rpat => rpat != null ? rpat : typeof(RenderPipelineAsset)).ToArray();

[Quality Audit] Make MenuItem dynamically hiddeable for LookDev and RenderGraphWindow [OnHold] #5519

[Quality Audit] Make MenuItem dynamically hiddeable for LookDev and RenderGraphWindow [OnHold] #5519

Uh oh!

Conversation

RSlysz commented Sep 2, 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 Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-vazquez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex-vazquez Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

RSlysz Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

alex-vazquez Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

RSlysz Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

alex-vazquez Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

RSlysz Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RSlysz commented Sep 2, 2021

Uh oh!

jenniferd-unity left a comment

Choose a reason for hiding this comment

Uh oh!

jenniferd-unity Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

alex-vazquez left a comment

Choose a reason for hiding this comment

Uh oh!

RSlysz commented Feb 25, 2022

Uh oh!

RSlysz commented Apr 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RSlysz commented Sep 2, 2021 •

edited

Loading

github-actions bot commented Sep 2, 2021 •

edited

Loading