Skip to content

Conversation

alex-vazquez
Copy link
Contributor

@alex-vazquez alex-vazquez commented Sep 21, 2021


Purpose of this PR

https://jira.unity3d.com/browse/XPIPELINE-327
https://jira.unity3d.com/browse/XPIPELINE-328

Warning for Volume Components from another pipeline:
image

Add an option to hide not overrided parameters for a component
image
image


Testing status

Describe what manual/automated tests were performed for this PR


Comments to reviewers

Notes for the reviewers you have assigned.

@alex-vazquez alex-vazquez requested a review from RSlysz September 21, 2021 10:26
@github-actions
Copy link

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_trunk
With changes to HDRP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_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:
/.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 alex-vazquez self-assigned this Sep 22, 2021
@alex-vazquez alex-vazquez requested review from a team and TheoWong-pixel September 22, 2021 10:01
Copy link
Contributor

@RSlysz RSlysz left a comment

Choose a reason for hiding this comment

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

Looks good but please rename the parameter for better code readability.


internal void InitShowOnlyOverridedParametersPreference()
{
string key = $"UI_Show_Only_Overrided_Parameters_{GetType()}";
Copy link
Contributor

Choose a reason for hiding this comment

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

It's sad that we cannot statically cache this or make it const. With inheritance, I can't see anyway to achieve it ;_;

}
}

void SetIsSupportedOnCurrentPipeline(VolumeComponent component, Type currentPipelineType)
Copy link
Contributor

Choose a reason for hiding this comment

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

[Readability] Either rename currentPipelineType to renderPipelineType or make the method find who is the current one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes for method name. It should be SetIsSupportedOnPipeline if the method don't get who is the current one

@alex-vazquez alex-vazquez marked this pull request as ready for review September 28, 2021 10:17
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.

Happened to notice a typo in user-facing text, so jumped in to let you know :)

else
menu.AddDisabledItem(EditorGUIUtility.TrTextContent("Show Additional Properties"));
menu.AddItem(EditorGUIUtility.TrTextContent("Show All Additional Properties..."), false, () => CoreRenderPipelinePreferences.Open());
menu.AddItem(EditorGUIUtility.TrTextContent("Show Only Overrided Parameters"), targetEditor.showOnlyOverridedParameters, () => targetEditor.showOnlyOverridedParameters = !targetEditor.showOnlyOverridedParameters);
Copy link
Contributor

Choose a reason for hiding this comment

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

Proper form is "overridden", not "overrided". Replace at least user-facing text, preferably also variable names.

@alex-vazquez alex-vazquez force-pushed the srp-workflows/volumes-multiple-pipelines branch from dea4b87 to 4d57758 Compare October 18, 2021 14:21
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.

Sorry to nitpick but "overriden" is still not correct, "overridden" with two d's is correct spelling.

@sebastienlagarde
Copy link
Contributor

Guess just run Yamato and merge?

@alex-vazquez
Copy link
Contributor Author

@sebastienlagarde we are going to put on hold this PR, until @fredericv-unity3d merge his PR as this will change drastically with his improvements.

@sebastienlagarde sebastienlagarde changed the title Srp workflows - Improvements working with Volume Components on a XPipeline Projects Srp workflows - Improvements working with Volume Components on a XPipeline Projects [Hold] Nov 4, 2021
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