Skip to content

NEW: Project-wide Actions#1735

Merged
jamesmcgill merged 34 commits intodevelopfrom
project-wide-actions-phase1
Aug 29, 2023
Merged

NEW: Project-wide Actions#1735
jamesmcgill merged 34 commits intodevelopfrom
project-wide-actions-phase1

Conversation

@jamesmcgill
Copy link
Copy Markdown
Collaborator

@jamesmcgill jamesmcgill commented Aug 22, 2023

Description

In-built set of Actions which is installed (currently inside InputManager.asset) and is available via InputSystem.actions
New UITK based editor installed in Project Settings provides a UI for editing these Actions

Changes made

  • New UITK Actions Editor (existing IMGUI editor is still used for normal .inputactions asset files)
  • New property InputSystem.actions backed by InputManager.asset and EditorBuildSettings for Players

Manual Testing

Testing Document

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
    • FogBugz is marked as "Resolved" with next release version correctly set.
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

Copy link
Copy Markdown
Collaborator

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

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

Looks ok overall (some questions in PR), I will check it out and run it too.

Some questions on bindings like we discussed in person. Am following up on those.

Lacks the Documentation .md file addition. Can you add that here too.


AssetDatabase.AddObjectToAsset(asset, s_AssetPath);

// Make sure all the elements in the asset have GUIDs and that they are indeed unique.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd be tempted to split this out into a support function taking the asset as a parameter.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In part because I think a 'restore defaults' functionality may use this later
https://jira.unity3d.com/browse/ISX-1564

}
}

// Create sub-asset for each action. This is so that users can select individual input actions from the asset when they're
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Similar to the above chunk of code I think this could be a support function

if (InputSystem.settings == null)
return;
m_OriginalPreloadedAssets = PlayerSettings.GetPreloadedAssets();
var preloadedAssets = PlayerSettings.GetPreloadedAssets();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could do this to avoid calling playersettings function twice as it may have higher cost.

var preloadedAssets = m_OriginalPreloadedAssets;

@@ -1,4 +1,4 @@
#if UNITY_EDITOR && UNITY_INPUT_SYSTEM_UI_TK_ASSET_EDITOR // We use some UITK controls that are only available in 2022.2 or later (MultiColumnListView for example)
#if UNITY_EDITOR && UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it worth keeping the comment?

s_projectWideActions, true);
}
#else
s_projectWideActions = Resources.FindObjectsOfTypeAll<InputActionAsset>().FirstOrDefault();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this line valid - could it end up picking up some other input action asset that we don't expect ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can the resources areas only contain the pre built one or could a user tuck some in there too ?
Would it be better to iterate over all and match by name.

// Hence we ignore adding temporary objects to preloaded assets.
if (!EditorUtility.IsPersistent(InputSystem.settings))
#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
EditorBuildSettings.TryGetConfigObject(InputSettingsProvider.kEditorBuildSettingsActionsConfigKey,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we reference config object here. We don't seem to use m_ProjectWideActions for anything

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh wait InputSystem.actions also pulls from the configobject.
In which case - why don't we just use that reference directly ?

(This might be ok, but it confused me a little, perhaps a little comment here would help if it remains the same)


public static void SaveAsset(SerializedObject serializedAsset, InputActionsEditorWindow currentWindow = null)
{
if ((focusedWindow == null || focusedWindow is not InputActionsEditorWindow) && currentWindow == null)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it ok this check is removed - I think Joao added this to make sure not used on the Input Asset flow where IMGUI still used. Perhaps its not really needed?

@Pauliusd01 Pauliusd01 self-requested a review August 24, 2023 11:21
lyndon-unity and others added 6 commits August 25, 2023 09:26
Next/Previous updated to be more intuative/same axis
Names for UI Click* restored to original
Added keyboard binding for Attack
Added some XR bindings

Some reordering too
E.g. so crouch/jump next to each other
This makes the diff slightly harder
…logies/InputSystem into project-wide-actions-phase1
Copy link
Copy Markdown
Collaborator

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

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

Approving to be mergable to develop. Testing will be continued in develop

Copy link
Copy Markdown
Collaborator

@jfreire-unity jfreire-unity left a comment

Choose a reason for hiding this comment

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

Approving so we can start testing.

@jamesmcgill
Copy link
Copy Markdown
Collaborator Author

jamesmcgill commented Aug 29, 2023

Merging in present state for pre-release testing
Known unrelated issues on trunk and 2023.2 with building player for jobs:

  • mac_standalone_il2cpp
  • android il2cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants