-
Notifications
You must be signed in to change notification settings - Fork 330
CHANGE: Deprecate useIMGUIEditorForAssets #2283
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
base: develop
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent |
Packages/com.unity.inputsystem/InputSystem/Actions/Composites/AxisComposite.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Actions/Composites/Vector2Composite.cs
Show resolved
Hide resolved
| // We simply assign the changed value back to the 'target' object. The input | ||
| // system will automatically detect a change in value. | ||
| target.scaleFactor = EditorGUILayout.Slider(m_ScaleFactorLabel, currentValue, 0, 2); | ||
| } |
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.
OnGUI is public, unfortunately. So we can't kill those in this PR without releasing a new major version.
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.
Minor: I am the sort of person that like a comment in the code in that case, exactly what you said "motivating it" instead of make it look like a mistake. E.g.
public override void OnGUI()
{
// Intentionally empty, OnGUI() is public and abstract so has to be implemented.
}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.
Just makes it quicker to review/look at since it activates less brain cells when looking at the same code 2 years later :)
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 hope we won't have these guys in 2 years from now, we should kill with the upcoming move to trunk
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.
But I agree. I'd prefer to have this landed though, but if you insist I'll go back and add the comment everywhere.
Packages/com.unity.inputsystem/InputSystem/Actions/Composites/AxisComposite.cs
Show resolved
Hide resolved
c5a98a1 to
9493240
Compare
Packages/com.unity.inputsystem/InputSystem/Editor/Analytics/InputBuildAnalytic.cs
Show resolved
Hide resolved
| } | ||
|
|
||
| // handled by OnDrawVisualElements with UI Toolkit | ||
| if (!InputSystem.settings.useIMGUIEditorForAssets) |
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.
Most likely this was a bug having the check this low, after the if block above: because it also deals with the imgui stuff really. The uitk path in this file seems to subscribe to changes in other way.
ekcoh
left a comment
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.
Great to see another review cleaning things up! Good work.
| // We simply assign the changed value back to the 'target' object. The input | ||
| // system will automatically detect a change in value. | ||
| target.scaleFactor = EditorGUILayout.Slider(m_ScaleFactorLabel, currentValue, 0, 2); | ||
| } |
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.
Minor: I am the sort of person that like a comment in the code in that case, exactly what you said "motivating it" instead of make it look like a mistake. E.g.
public override void OnGUI()
{
// Intentionally empty, OnGUI() is public and abstract so has to be implemented.
}| // We simply assign the changed value back to the 'target' object. The input | ||
| // system will automatically detect a change in value. | ||
| target.scaleFactor = EditorGUILayout.Slider(m_ScaleFactorLabel, currentValue, 0, 2); | ||
| } |
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.
Just makes it quicker to review/look at since it activates less brain cells when looking at the same code 2 years later :)
|
|
||
| ### Changed | ||
| - Project-Wide Input Actions support can no longer be disabled (removed the UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS define). (ISX-2397) | ||
| - Project-Wide Input Actions support can no longer compiled out (removed the UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS define). (ISX-2397) |
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.
Minor: UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS (fixed/code formatting style)
Packages/com.unity.inputsystem/InputSystem/Editor/Analytics/InputBuildAnalytic.cs
Show resolved
Hide resolved
| private GUIContent[] m_ParameterLabels; | ||
|
|
||
| // .Item1 is text, .Item2 is tooltip | ||
| private Tuple<string, string>[] m_ParameterLabels; |
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.
Might this even be ValueTuple to reduce object allocation?
|
|
||
| // Create parameter labels. | ||
| m_ParameterLabels = new GUIContent[m_Parameters.Length]; | ||
| m_ParameterLabels = new Tuple<string, string>[m_Parameters.Length]; |
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.
Might this even be value tuple to reduce object allocation?
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.
Yeah I think you're right, I could do that too
ekcoh
left a comment
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.
Marking this as "request changes" only for sake of typo in identifier name.
Co-authored-by: Håkan Sidenvall <[email protected]>
Description
Here we deprecate the useIMGUIEditorForAssets feature flag.
The aim is to reduce total variability caused by having to keep code that can worth both ways - with uitk and without. We've had the uitk code running for a while now, and with the minimum supported Editor 22.3 TLS we no longer have to care about the case when uitk wasn't available.
Testing status & QA
Local testing
Overall Product Risks
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.