FIX: some actions not being triggered when other duplicate control bindings exist (ISXB-254).#1600
Conversation
jamesmcgill
changed the title
jamesmcgill
changed the title
|
My take on this is that if we're making it opt-in now, we should make it part of the input system settings and add a checkbox to the project settings dialogue to allow switching it on globally. That also gives us a chance to put a little warning message in there about the repercussions should you have multiple action maps enabled at the same time with multiple actions using the same bindings. If we don't go this way, then I'd suggest changing the wording in the changelog from "Reverted" to "Changed to opt-in" or similar. When I see reverted I think the behaviour has been removed entirely. |
jimon
left a comment
jimon
left a comment
There was a problem hiding this comment.
LGTM. input settings logic feels a bit error prone though
|
|
||
| // REMOVE: this is a temporary crutch to disable shortcut support by default but while also preserving the | ||
| // existing flag name, as users are aware of that now. | ||
| if (featureName == InputFeatureNames.kDisableShortcutSupport) |
There was a problem hiding this comment.
What would happen if we simply removed the internal feature flag? If users are using it, it's to turn off the feature, but now it's opt in, so if we remove it, their code will have the same behaviour.
There was a problem hiding this comment.
That's true. I was thinking that there would be no programmatic way to enable/disable this until we add the public property. But your right, only users who were disabling it before should know about the existence of this and it won't hurt them that it does nothing now.
There was a problem hiding this comment.
I'm going to leave it in for v1.4.4. There are some tests for shortcuts that I need to enable this programmatically somehow.
| EditorGUILayout.HelpBox("Please note that enabling Improved Shortcut Support will cause actions to consume and block any other actions which are enabled and sharing the same controls. " | ||
| + "E.g. when pressing the 'Shift+B' keys, the associated action would trigger but any action bound to just the 'B' key would be prevented from triggering at the same time, which would be the expected result in this example. " | ||
| + "However, in other cases this might not give the desired result. E.g. if the 'W', 'A', 'S' and 'D' keys are bound to different actions which are all enabled at the same time, then only one of those actions will trigger. " | ||
| + "Therefore if you enable this feature, be sure to remove any of these duplicate control bindings so that controls are only ever bound to a single action. Alternatively, you can disable actions or action maps at runtime to remove these conflicts." | ||
| , MessageType.None); |
There was a problem hiding this comment.
Some suggestions:
- "Please note that enabling Improved Shortcut Support will cause actions with composite bindings to consume input and block..."
- Could point out that blocking occurs even across action maps. I don't think users are really often aware of where the actions live and just enable entire assets.
- I feel we can leave out the repeated instruction about Shift-B and B since that is more about the features intended purpose and is already in the tooltip.
- Would this benefit from a more detailed worked example describing two action maps, one for UI and one for Gameplay, where both use the up, down, left, and right keys (pretty common one on the forums) and maybe some other action that will be lower priority and only uses left and right keys for example.
- It would help users to know how priority is decided between actions. Should we include some instructions on that, i.e. number of composite parts determines priority but actions with the same number of parts are non-deterministic?
- Is the advice really to remove duplicate controls? I like the second bit about disabling actions but maybe we could expand it to something like "Since event consumption only occurs for enabled actions, you can resolve most unexpected issues by ensuring that only those action maps that are relevant to your game's current context are enabled. Enable or disable actions as your game or application moves between different contexts."
There was a problem hiding this comment.
Tried to reword this incorporating your suggestions
4 participants
Description
In v1.4 we improved shortcut support by having actions consuming their inputs. This meant, for example, that SHIFT+B would trigger it's action but any action bound to the 'B' key would be blocked.
However we have been receiving many reports that users are not getting the expected behaviour in their projects that may have unknowingly (or knowingly) multiple actions bound to the same controls.
E.g. the issues with WASD keys, with the UIInputModule Navigate action conflicting with Player controls, if both action maps were enabled and many other similar cases that were reported.
This PR restores the previous behaviour by default and makes the the new event consumption behaviour (shortcut support) opt-in now and accessible as a toggle via project settings.
Changes made
Added a toggle in project settings with the default value
false.Modified the
SetInternalFeatureFlagandIsFeatureEnabledto use this new value when queried forkDisableShortcutSupport. It was deliberately done this way to preserve the current (non-public) API, as users are aware of it now. Rather than doing the natural thing of changingkDisableShortcutSupporttokEnableShortcutSupportor removing it entirely, which would have been cleaner.Notes
InputSystem.settings.SetInternalFeatureFlag("DISABLE_SHORTCUT_SUPPORT", true);is still available here as some users are aware of it now, but a deprecation warning is being emitted.The setting a private field, but it does need to be saved in the asset.
I am preparing a v1.5 version of this with that removed and cleans up the settings code with proper api.
Checklist
Before review:
Changed,Fixed,Addedsections.([case %number%](https://issuetracker.unity3d.com/issues/...)).Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.