-
Notifications
You must be signed in to change notification settings - Fork 855
Srp workflows - Debug panel system to core #5726
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.
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
Srp workflows - Debug panel system to core #5726
Conversation
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. URP SRP Core 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not want to block the PR but there are somethings that can be improved on the framework itself.
namespace UnityEngine.Rendering | ||
{ | ||
/// <summary> | ||
/// Templated class for <see cref="IDebugDisplaySettings"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should describe how to use the class. Like: "Subclass this to benefit for commonly used features regarding IDebugDisplaySettings"
{ | ||
protected readonly HashSet<IDebugDisplaySettingsData> m_Settings = new HashSet<IDebugDisplaySettingsData>(); | ||
|
||
private static readonly Lazy<T> s_Instance = new Lazy<T>(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to clear the data when entering or exiting playmode?
Note: When domain reload is not performed, the C# statics are not cleared and can contain invalid values.
namespace UnityEngine.Rendering.Universal | ||
namespace UnityEngine.Rendering | ||
{ | ||
public abstract class DebugDisplaySettingsPanel : IDebugDisplaySettingsPanelDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class is disposable, although it can still be used while disposed. Is it intended?
@Unity-Technologies/gfx-qa-urp Please do a smoke test to the Rendering Debugger in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, need QA to verify that everything works the same way before and after this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everything is working the same way as it was before the changes. there are some issues unrelated to this PR that I will report
Purpose of this PR
https://jira.unity3d.com/browse/XPIPELINE-264
Moving all Debug Panel framework to Core in order to be able to implement the Volumes Panel on URP.
Testing status
Testing the URP debug panels work in the same way
Comments to reviewers
Simply doing a templated class with common functinality for URP and HDRP and move all the interfaces to Core.