-
Notifications
You must be signed in to change notification settings - Fork 323
FIX: Custom processors serialises enum by index rather than by value. #2164
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?
Changes from all commits
6504c53
e9cff62
7284150
73890bd
9020c5e
9819aab
43278ad
a06e9d3
580e931
774190a
6d50a6b
7c7b927
a521b32
8c65e40
37d1dfb
8bc3ef0
359f03d
b41abf8
8d168c8
a81282c
b3f63c9
40ce21e
261cbd6
42bb20c
8acf7fe
51994b7
fbbacad
101dd7c
1f6bd45
42807fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
#if UNITY_EDITOR && UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS && UNITY_6000_0_OR_NEWER | ||
|
||
using System; | ||
using NUnit.Framework; | ||
using System.Collections; | ||
using System.Linq; | ||
using UnityEditor; | ||
using UnityEngine; | ||
using UnityEngine.InputSystem; | ||
using UnityEngine.InputSystem.Editor; | ||
using UnityEngine.TestTools; | ||
using UnityEngine.UIElements; | ||
|
||
internal enum SomeEnum | ||
{ | ||
OptionA = 10, | ||
AswinRajGopal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
OptionB = 20 | ||
} | ||
|
||
#if UNITY_EDITOR | ||
[InitializeOnLoad] | ||
#endif | ||
internal class CustomProcessor : InputProcessor<float> | ||
{ | ||
public SomeEnum SomeEnum; | ||
|
||
#if UNITY_EDITOR | ||
static CustomProcessor() | ||
{ | ||
Initialize(); | ||
} | ||
|
||
#endif | ||
|
||
[RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] | ||
private static void Initialize() | ||
{ | ||
InputSystem.RegisterProcessor<CustomProcessor>(); | ||
} | ||
|
||
public override float Process(float value, InputControl control) | ||
{ | ||
return value; | ||
} | ||
} | ||
|
||
internal class CustomProcessorEnumTest : UIToolkitBaseTestWindow<InputActionsEditorWindow> | ||
AswinRajGopal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
InputActionAsset m_Asset; | ||
|
||
public override void OneTimeSetUp() | ||
{ | ||
base.OneTimeSetUp(); | ||
m_Asset = AssetDatabaseUtils.CreateAsset<InputActionAsset>(); | ||
|
||
var actionMap = m_Asset.AddActionMap("Action Map"); | ||
|
||
actionMap.AddAction("Action", InputActionType.Value, processors: "Custom(SomeEnum=10)"); | ||
} | ||
|
||
public override void OneTimeTearDown() | ||
{ | ||
AssetDatabaseUtils.Restore(); | ||
base.OneTimeTearDown(); | ||
} | ||
|
||
public override IEnumerator UnitySetup() | ||
{ | ||
m_Window = InputActionsEditorWindow.OpenEditor(m_Asset); | ||
yield return base.UnitySetup(); | ||
} | ||
|
||
[UnityTest] | ||
public IEnumerator ProcessorEnum_ShouldSerializeByValue_WhenSerializedToAsset() | ||
{ | ||
// Serialize current asset to JSON, and check that initial JSON contains default enum value for OptionA | ||
var json = m_Window.currentAssetInEditor.ToJson(); | ||
|
||
Assert.That(json.Contains("Custom(SomeEnum=10)"), Is.True, | ||
"Serialized JSON does not contain the expected custom processor string for OptionA."); | ||
|
||
// Query the dropdown with exactly two enum choices and check that the drop down is present in the UI | ||
var dropdownList = m_Window.rootVisualElement.Query<DropdownField>().Where(d => d.choices.Count == 2).ToList(); | ||
Assume.That(dropdownList.Count > 0, Is.True, "Enum parameter dropdown not found in the UI."); | ||
|
||
// Determine the new value to be set in the dropdown, focus the dropdown before dispatching the change | ||
var dropdown = dropdownList.First(); | ||
var newValue = dropdown.choices[1]; | ||
dropdown.Focus(); | ||
dropdown.value = newValue; | ||
|
||
// Create and send a change event from OptionA to OptionB | ||
var changeEvent = ChangeEvent<Enum>.GetPooled(SomeEnum.OptionA, SomeEnum.OptionB); | ||
changeEvent.target = dropdown; | ||
dropdown.SendEvent(changeEvent); | ||
|
||
// Find the save button in the window, focus and click the save button to persist the changes | ||
var saveButton = m_Window.rootVisualElement.Q<Button>("save-asset-toolbar-button"); | ||
Assume.That(saveButton, Is.Not.Null, "Save Asset button not found in the UI."); | ||
saveButton.Focus(); | ||
SimulateClickOn(saveButton); | ||
|
||
Assert.That(dropdown.value, Is.EqualTo(newValue)); | ||
|
||
// Verify that the updated JSON contains the new enum value for OpitonB | ||
var updatedJson = m_Window.currentAssetInEditor.ToJson(); | ||
Assert.That(updatedJson.Contains("Custom(SomeEnum=20)"), Is.True, "Serialized JSON does not contain the updated custom processor string for OptionB."); | ||
|
||
yield return null; | ||
} | ||
} | ||
#endif |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,9 @@ | ||
using System; | ||
using System.Collections; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Reflection; | ||
using UnityEngine.InputSystem.Editor; | ||
using UnityEngine.InputSystem.Utilities; | ||
|
||
////TODO: make the FindAction logic available on any IEnumerable<InputAction> and IInputActionCollection via extension methods | ||
|
@@ -275,6 +278,21 @@ | |
return action; | ||
} | ||
} | ||
/// <summary> | ||
/// File‐format version constants for InputActionAsset JSON. | ||
/// </summary> | ||
static class JsonVersion | ||
{ | ||
/// <summary>The original JSON version format for InputActionAsset.</summary> | ||
public const int Version0 = 0; | ||
|
||
/// <summary>Updated JSON version format for InputActionAsset.</summary> | ||
/// <remarks>Changes representation of parameter values from being serialized by value to being serialized by value.</remarks> | ||
public const int Version1 = 1; | ||
|
||
/// <summary>The current version.</summary> | ||
public const int Current = Version1; | ||
} | ||
|
||
/// <summary> | ||
/// Return a JSON representation of the asset. | ||
|
@@ -296,8 +314,10 @@ | |
/// <seealso cref="FromJson"/> | ||
public string ToJson() | ||
{ | ||
var hasContent = m_ActionMaps.LengthSafe() > 0 || m_ControlSchemes.LengthSafe() > 0; | ||
return JsonUtility.ToJson(new WriteFileJson | ||
{ | ||
version = hasContent ? JsonVersion.Current : JsonVersion.Version0, | ||
name = name, | ||
maps = InputActionMap.WriteFileJson.FromMaps(m_ActionMaps).maps, | ||
controlSchemes = InputControlScheme.SchemeJson.ToJson(m_ControlSchemes), | ||
|
@@ -379,6 +399,7 @@ | |
throw new ArgumentNullException(nameof(json)); | ||
|
||
var parsedJson = JsonUtility.FromJson<ReadFileJson>(json); | ||
MigrateJson(ref parsedJson); | ||
parsedJson.ToAsset(this); | ||
} | ||
|
||
|
@@ -950,6 +971,7 @@ | |
[Serializable] | ||
internal struct WriteFileJson | ||
{ | ||
public int version; | ||
AswinRajGopal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public string name; | ||
public InputActionMap.WriteMapJson[] maps; | ||
public InputControlScheme.SchemeJson[] controlSchemes; | ||
|
@@ -965,6 +987,7 @@ | |
[Serializable] | ||
internal struct ReadFileJson | ||
{ | ||
public int version; | ||
AswinRajGopal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public string name; | ||
public InputActionMap.ReadMapJson[] maps; | ||
public InputControlScheme.SchemeJson[] controlSchemes; | ||
|
@@ -981,5 +1004,73 @@ | |
map.m_Asset = asset; | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// If parsedJson.version is older than Current, rewrite every | ||
/// action.processors entry to replace “enumName(Ordinal=…)” with | ||
/// “enumName(Value=…)” and bump parsedJson.version. | ||
/// </summary> | ||
internal void MigrateJson(ref ReadFileJson parsedJson) | ||
{ | ||
if (parsedJson.version >= JsonVersion.Version1) | ||
return; | ||
if ((parsedJson.maps?.Length ?? 0) > 0 && (parsedJson.version) < JsonVersion.Version1) | ||
{ | ||
for (var mi = 0; mi < parsedJson.maps.Length; ++mi) | ||
{ | ||
var mapJson = parsedJson.maps[mi]; | ||
for (var ai = 0; ai < mapJson.actions.Length; ++ai) | ||
{ | ||
var actionJson = mapJson.actions[ai]; | ||
var raw = actionJson.processors; | ||
if (string.IsNullOrEmpty(raw)) | ||
continue; | ||
|
||
AswinRajGopal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var list = NameAndParameters.ParseMultiple(raw).ToList(); | ||
var rebuilt = new List<string>(list.Count); | ||
foreach (var nap in list) | ||
{ | ||
var procType = InputSystem.TryGetProcessor(nap.name); | ||
if (nap.parameters.Count == 0 || procType == null) | ||
{ | ||
rebuilt.Add(nap.ToString()); | ||
continue; | ||
Check warning on line 1037 in Packages/com.unity.inputsystem/InputSystem/Actions/InputActionAsset.cs
|
||
} | ||
|
||
var dict = nap.parameters.ToDictionary(p => p.name, p => p.value.ToString()); | ||
var anyChanged = false; | ||
foreach (var field in procType.GetFields(BindingFlags.Public | BindingFlags.Instance).Where(f => f.FieldType.IsEnum)) | ||
{ | ||
if (dict.TryGetValue(field.Name, out var ordS) && int.TryParse(ordS, out var ord)) | ||
{ | ||
var values = Enum.GetValues(field.FieldType).Cast<object>().ToArray(); | ||
if (ord >= 0 && ord < values.Length) | ||
{ | ||
dict[field.Name] = Convert.ToInt32(values[ord]).ToString(); | ||
anyChanged = true; | ||
} | ||
} | ||
} | ||
Check warning on line 1053 in Packages/com.unity.inputsystem/InputSystem/Actions/InputActionAsset.cs
|
||
|
||
if (!anyChanged) | ||
{ | ||
rebuilt.Add(nap.ToString()); | ||
} | ||
Check warning on line 1058 in Packages/com.unity.inputsystem/InputSystem/Actions/InputActionAsset.cs
|
||
else | ||
{ | ||
var paramText = string.Join(",", dict.Select(kv => $"{kv.Key}={kv.Value}")); | ||
rebuilt.Add($"{nap.name}({paramText})"); | ||
} | ||
} | ||
Check warning on line 1064 in Packages/com.unity.inputsystem/InputSystem/Actions/InputActionAsset.cs
|
||
|
||
actionJson.processors = string.Join(";", rebuilt); | ||
mapJson.actions[ai] = actionJson; | ||
} | ||
Check warning on line 1068 in Packages/com.unity.inputsystem/InputSystem/Actions/InputActionAsset.cs
|
||
parsedJson.maps[mi] = mapJson; | ||
} | ||
} | ||
// Bump the version so we never re-migrate | ||
parsedJson.version = JsonVersion.Version1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be JSonVersion.Current There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my perspective this looks correct if this migration step only handles v0 to v1. It would need to be followed by a v1 to v2 if Current was e.g. 2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I guess I can see that, I was expecting that after running this function we would always end up on the latest version anyways, and we would always need to remember to update the version here... but its a nitpick There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
{ | ||
"version": 1, | ||
"name": "DefaultInputActions", | ||
"maps": [ | ||
{ | ||
|
Uh oh!
There was an error while loading. Please reload this page.