Skip to content

XPipeline- Improvements of Light UI inspector code. #5367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Sep 17, 2021
Merged

XPipeline- Improvements of Light UI inspector code. #5367

merged 4 commits into from
Sep 17, 2021

Conversation

alex-vazquez
Copy link
Contributor

@alex-vazquez alex-vazquez commented Aug 17, 2021 •
edited
Loading

Purpose of this PR

XPIPELINE-247

HDRP and URP look the same now:

image

Testing status

  • Check all the icons
  • Check presets work on URP

Comments to reviewers

Moving code to Core from HDRP and using it on URP.

@alex-vazquez alex-vazquez self-assigned this Aug 17, 2021
@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_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.

arttu-peltonen
arttu-peltonen reviewed Aug 18, 2021
View reviewed changes
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.

Found some code that could potentially get removed?

@arttu-peltonen
Copy link
Contributor

General comment: I haven't found a great way to get a sense of "what's changed" when reviewing refactoring PRs such as this one, so any tips with this are appreciated. Lots of things have just moved to new files, but sometimes there are subtle changes that are hard to notice. Some ideas that might help with this issue in the future:

  1. List non-functional changes separately from the functional changes in the PR description.
  2. Make separate commits where things are only moved around, and functional changes in other commits. This would allow me to view the functional changes in isolation.

Any thoughts?

@martint-unity
Copy link
Contributor

Why do we have a drop-down for only one option?
And also the length of the drop-down is wider than that it needs to be. SO it sticks out on the right hand side. Just looks a bit off.
image

martint-unity
martint-unity suggested changes Aug 18, 2021
View reviewed changes
Copy link
Contributor

@martint-unity martint-unity left a comment

Choose a reason for hiding this comment

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

Have a few comments

com.unity.render-pipelines.core/Editor/Lighting/LightUI.Skin.cs Outdated
public static readonly GUIContent lightLayer = EditorGUIUtility.TrTextContent("Light Layer", "Specifies the current Light Layers that the Light affects. This Light illuminates corresponding Renderers with the same Light Layer flags.");

// in casse that you want to keep the indentation but have nothing to write
public static readonly GUIContent empty = EditorGUIUtility.TrTextContent(" ");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the place for this empty style. Isn't this a more global thing? Or maybe I am missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved in another PR

com.unity.render-pipelines.core/Editor/Lighting/LightUnit/LightUnitSliderSettings.cs Outdated
return CoreEditorUtils.LoadIcon(@"Packages/com.unity.render-pipelines.core/Editor/Lighting/Icons/LightUnitIcons", name, ".png");
}

// TODO: Move all light unit icons from the package into the built-in resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a Jira ticket for this task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

XPIPELINE-247

com.unity.render-pipelines.high-definition/Editor/Lighting/LightUnit/HDLightUnitSlider.cs Outdated
}

// Need to cache the serialized object on the slider, to add support for the preset selection context menu (need to apply changes to serialized)
// TODO: This slider drawer is getting kind of bloated. Break up the implementation into where it is actually used?
Copy link
Contributor

Choose a reason for hiding this comment

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

is this TODO tracked as a JIRA somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not that I know

RSlysz
RSlysz approved these changes Aug 30, 2021
View reviewed changes
@alex-vazquez alex-vazquez force-pushed the x-pipeline/refactor-light-ui branch from 7ee78db to 03847e7 Compare September 6, 2021 09:54
martint-unity
martint-unity suggested changes Sep 6, 2021
View reviewed changes
@alex-vazquez alex-vazquez marked this pull request as ready for review September 8, 2021 13:07
@alex-vazquez alex-vazquez requested review from a team as code owners September 8, 2021 13:07
erikabar
erikabar approved these changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@erikabar erikabar left a comment

Choose a reason for hiding this comment

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

Developer already tested this PR, I'm approving without additional testing

@alex-vazquez alex-vazquez force-pushed the x-pipeline/refactor-light-ui branch from a67ee51 to 5b2bfb4 Compare September 17, 2021 07:33
@alex-vazquez alex-vazquez merged commit 237bc7c into master Sep 17, 2021
@alex-vazquez alex-vazquez deleted the x-pipeline/refactor-light-ui branch September 17, 2021 13:08
@alex-vazquez alex-vazquez mentioned this pull request Sep 20, 2021
Merged
@arttu-peltonen arttu-peltonen mentioned this pull request Oct 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RSlysz RSlysz RSlysz approved these changes

@arttu-peltonen arttu-peltonen arttu-peltonen approved these changes

+2 more reviewers

@martint-unity martint-unity martint-unity approved these changes

@erikabar erikabar erikabar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alex-vazquez alex-vazquez

Labels

backported-2021.2 HDRP SRP universal

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@alex-vazquez @arttu-peltonen @martint-unity @RSlysz @erikabar @sebastienlagarde @alex-vazquez-unity3d