Skip to content

Fix ISXB-1794: Processor (and interaction) list use composite part va… #2378

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

Open
K-Tone wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix ISXB-1794: Processor (and interaction) list use composite part va… #2378

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/Selectors.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,9 +311,7 @@

public static IEnumerable<ParameterListView> GetInteractionsAsParameterListViews(InputActionsEditorState state, SerializedInputAction? inputAction)
{
Type expectedValueType = null;
if (inputAction.HasValue && !string.IsNullOrEmpty(inputAction.Value.expectedControlType))
expectedValueType = EditorInputControlLayoutCache.GetValueType(inputAction.Value.expectedControlType);
var expectedValueType = GetExpectedValueTypeForProcessorsOrInteractions(state, inputAction);

var interactions = string.Empty;
if (inputAction.HasValue && state.selectionType == SelectionType.Action)
Expand All @@ -328,13 +326,35 @@
InputInteraction.GetValueType);
}

public static IEnumerable<ParameterListView> GetProcessorsAsParameterListViews(InputActionsEditorState state, SerializedInputAction? inputAction)
/// <summary>
/// Expected value type for the current selection. When the selection is a composite part binding,
/// returns the part's value type (e.g. float) instead of the composite's (e.g. Vector2). ISXB-1794.
/// </summary>
private static Type GetExpectedValueTypeForProcessorsOrInteractions(InputActionsEditorState state, SerializedInputAction? inputAction)
Copy link
Contributor

@u-pr u-pr bot Mar 12, 2026

Choose a reason for hiding this comment

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

low

The method name GetExpectedValueTypeForProcessorsOrInteractions is quite specific to its current callers. To improve maintainability, consider a more generic name like GetExpectedValueTypeForSelection. This would better reflect the method's logic and make it more reusable if other parts of the editor need to resolve the expected value type in the future.

🤖 Helpful? 👍/👎

Copy link
Collaborator

@ekcoh ekcoh Mar 12, 2026

Choose a reason for hiding this comment

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

Makes sense, this isn't the best function name, but it is at least not ambiguous.

{
var processors = string.Empty;
Type expectedValueType = null;
if (state.selectionType == SelectionType.Binding)
{
var binding = GetSelectedBinding(state);
if (binding.HasValue && binding.Value.isPartOfComposite)
{
var compositeName = NameAndParameters.Parse(binding.Value.compositePath).name;
Copy link
Contributor

@u-pr u-pr bot Mar 12, 2026

Choose a reason for hiding this comment

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

medium

If the input action asset is slightly malformed or corrupted (e.g., a binding has isPartOfComposite set to true, but lacks a valid parent composite binding), binding.Value.compositePath could potentially be null.

Since NameAndParameters.Parse explicitly throws an ArgumentNullException when passed a null string, this would result in an unhandled exception and break the editor UI when selecting that binding.

To make the editor more robust, consider using the null-coalescing operator to ensure a safe fallback:

var compositeName = NameAndParameters.Parse(binding.Value.compositePath ?? string.Empty).name;

🤖 Helpful? 👍/👎

Copy link
Collaborator

@ekcoh ekcoh Mar 12, 2026

Choose a reason for hiding this comment

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

Makes sense, the UI could be exposed to any asset so adding robustness to it makes sense. @u-pr can you apply the suggested change to this PR?

var partName = binding.Value.name;
var expectedLayout = InputBindingComposite.GetExpectedControlLayoutName(compositeName, partName);
if (!string.IsNullOrEmpty(expectedLayout))
return EditorInputControlLayoutCache.GetValueType(expectedLayout);
}
}

Check warning on line 346 in Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/Selectors.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/Selectors.cs#L336-L346 

Added lines #L336 - L346 were not covered by tests

if (inputAction.HasValue && !string.IsNullOrEmpty(inputAction.Value.expectedControlType))
expectedValueType = EditorInputControlLayoutCache.GetValueType(inputAction.Value.expectedControlType);
return EditorInputControlLayoutCache.GetValueType(inputAction.Value.expectedControlType);

return null;
}

public static IEnumerable<ParameterListView> GetProcessorsAsParameterListViews(InputActionsEditorState state, SerializedInputAction? inputAction)
{
var processors = string.Empty;
var expectedValueType = GetExpectedValueTypeForProcessorsOrInteractions(state, inputAction);

if (inputAction.HasValue && state.selectionType == SelectionType.Action)
processors = inputAction.Value.processors;
Expand Down
Loading