Skip to content

Using variable size IME events #1628

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions Assets/Tests/InputSystem/APIVerificationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,14 @@ public class SwitchProControllerHID : UnityEngine.InputSystem.Gamepad
[Property("Exclusions", @"1.0.0
public class DualShock4GamepadHID : UnityEngine.InputSystem.DualShock.DualShockGamepad
")]
// IMECompositionEvent added unsafe attribute
[Property("Exclusions", @"1.0.0
public struct IMECompositionEvent : UnityEngine.InputSystem.LowLevel.IInputEventTypeInfo
")]
// IMECompositionEvent.compositionString changed from field to property due to hard-to-avoid changes to IMECompositionString struct
[Property("Exclusions", @"1.0.0
public UnityEngine.InputSystem.LowLevel.IMECompositionString compositionString;
")]
public void API_MinorVersionsHaveNoBreakingChanges()
{
var currentVersion = CoreTests.PackageJson.ReadVersion();
Expand Down
26 changes: 24 additions & 2 deletions Assets/Tests/InputSystem/CoreTests_Devices.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5235,9 +5235,31 @@ public void Devices_CanListenForIMECompositionEvents()
Assert.AreEqual(composition.ToString(), imeCompositionCharacters);
};

var inputEvent = IMECompositionEvent.Create(keyboard.deviceId, imeCompositionCharacters,
IMECompositionEventVariableSize.QueueEvent(keyboard.deviceId, imeCompositionCharacters,
InputRuntime.s_Instance.currentTime);
InputSystem.Update();

Assert.That(callbackWasCalled, Is.True);
}

[Test]
[Category("Devices")]
public void Devices_CanReadEmptyIMECompositionEvents()
{
const string imeCompositionCharacters = "";
var callbackWasCalled = false;

var keyboard = InputSystem.AddDevice<Keyboard>();
keyboard.onIMECompositionChange += composition =>
{
Assert.That(callbackWasCalled, Is.False);
callbackWasCalled = true;
Assert.AreEqual(composition.Count, 0);
Assert.AreEqual(composition.ToString(), imeCompositionCharacters);
};

IMECompositionEventVariableSize.QueueEvent(keyboard.deviceId, imeCompositionCharacters,
InputRuntime.s_Instance.currentTime);
InputSystem.QueueEvent(ref inputEvent);
InputSystem.Update();

Assert.That(callbackWasCalled, Is.True);
Expand Down
2 changes: 1 addition & 1 deletion Packages/com.unity.inputsystem/Documentation~/Events.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ There are three types of Device events:
There are two types of text events:

* [`TextEvent`](../api/UnityEngine.InputSystem.LowLevel.TextEvent.html) (`'TEXT'`)
* [`IMECompositionEvent`](../api/UnityEngine.InputSystem.LowLevel.IMECompositionEvent.html) (`'IMES'`)
* [`IMECompositionEventVariableSize`](../api/UnityEngine.InputSystem.LowLevel.IMECompositionEventVariableSize.html) (`'IMEC'`)

## Working with events

Expand Down
4 changes: 4 additions & 0 deletions Packages/com.unity.inputsystem/Documentation~/filter.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
apiRules:
- include:
# IMECompositionEvent is marked as obsolete
uid: UnityEngine.InputSystem.LowLevel.IMECompositionEvent
type: Type
- exclude:
# inherited Object methods
uidRegex: ^System\.Object\..*$
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace UnityEngine.InputSystem.LowLevel
/// input through <see cref="TextEvent"/>.
/// </remarks>
/// <seealso cref="TextEvent"/>
/// <seealso cref="IMECompositionEvent"/>
/// <seealso cref="IMECompositionEventVariableSize"/>
public interface ITextInputReceiver
{
/// <summary>
Expand All @@ -32,7 +32,7 @@ public interface ITextInputReceiver
/// Called when an IME composition is in-progress or finished.
/// </summary>
/// <param name="compositionString">The current composition.</param>
/// <seealso cref="IMECompositionEvent"/>
/// <seealso cref="IMECompositionEventVariableSize"/>
/// <seealso cref="Keyboard.onIMECompositionChange"/>
/// <remarks>
/// The method will be repeatedly called with the current string while composition is in progress.
Expand Down
185 changes: 137 additions & 48 deletions Packages/com.unity.inputsystem/InputSystem/Events/IMECompositionEvent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,117 @@
using System.Collections;
using System.Collections.Generic;
using System.Runtime.InteropServices;
using Unity.Collections;
using Unity.Collections.LowLevel.Unsafe;
using UnityEngine.InputSystem.Utilities;

//// TODO for v2 remove and replace with just string.

namespace UnityEngine.InputSystem.LowLevel
{
/// <summary>
/// A specialized event that contains the current IME Composition string, if IME is enabled and active.
/// This event contains the entire current string to date, and once a new composition is submitted will send a blank string event.
/// Deprecated variant of IME composition event. Please use <see cref="IMECompositionEventVariableSize"/> for replacement.
/// </summary>
[Obsolete("Use IMECompositionEventVariableSize instead.")]
[StructLayout(LayoutKind.Explicit, Size = InputEvent.kBaseEventSize + sizeof(int) + (sizeof(char) * kIMECharBufferSize))]
public struct IMECompositionEvent : IInputEventTypeInfo
public unsafe struct IMECompositionEvent : IInputEventTypeInfo
{
// These needs to match the native ImeCompositionStringInputEventData settings
internal const int kIMECharBufferSize = 64;
private const int kIMECharBufferSize = 64;
public const int Type = 0x494D4553;

[FieldOffset(0)]
public InputEvent baseEvent;

[FieldOffset(InputEvent.kBaseEventSize)]
public IMECompositionString compositionString;
private int length;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if instead of breaking the API here, we leave IMECompositionString with the same data layout as before, but when the string is greater than kIMECharBufferSize characters long, we (ab)use the first 8 bytes of the fixed buffer to point at a string in some centrally managed storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would need some versioning then, which is not optimal. I would really prefer to use a normal managed string for such case and not reinvent C++ in C#. Question is more how not to break API here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, using a normal string is the "right" way, but we can't do that and not break the API, so I see the choice as delay this PR until version 2 or make a hack like this that would keep the original data layout. I'm not sure what you mean by versioning? I don't see where we'd need something like that.

Copy link
Contributor Author

@jimon jimon Jan 18, 2023

Choose a reason for hiding this comment

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

From my undertraining a managed referenced can't be aliased with unmanaged memory, otherwise GC can't collect it.
So because IMECompositionString is not IDisposable, we can't have a finalizer on it, meaning it would need to point to some other place that stores a string.

In that case lifetime of place storing a string and potential usages of it are uncorrelated, meaning someone could store a copy of IMECompositionString for a long time, and then query it's content.

For such cases a standard solution in C++ is to store a pointer + version of the data, and if handle has different version than a central location, then data is lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm leaning towards IMECompositionString2 and calling it a day, we can clean up when mystical version 2 will come if ever.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMECompositionString2 would be problematic because of Keyboard. I don't think we can do that, unless you'd also have Keyboard.onIMECompositionChangeV2.

Yep, I'm saying let it point to some other place. Actually it wouldn't even have to be a pointer, just an integer lookup into some table of strings. And there's no real issue of versioning. Access would be strictly through the IMECompositionString, and lets just make it IDisposable (we need that anyway because of the issue I mentioned above about building a string from a pointer. Don't think that makes a copy.). You might need to do a swap back or something on disposal and update the index in the affected composition string, but that should be fine.


[FieldOffset(InputEvent.kBaseEventSize + sizeof(int))]
private fixed char buffer[kIMECharBufferSize];

public IMECompositionString compositionString
{
get
{
fixed(char* ptr = buffer)
return new IMECompositionString(ptr, length);
}
}

public FourCC typeStatic => Type;

public static IMECompositionEvent Create(int deviceId, string compositionString, double time)
{
var inputEvent = new IMECompositionEvent();
inputEvent.baseEvent = new InputEvent(Type, InputEvent.kBaseEventSize + sizeof(int) + (sizeof(char) * kIMECharBufferSize), deviceId, time);
inputEvent.compositionString = new IMECompositionString(compositionString);
inputEvent.length = compositionString.Length > kIMECharBufferSize ? kIMECharBufferSize : compositionString.Length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't immediately clear to me that this was a clamp. Thought it was a bug at first.

inputEvent.length = Math.Min(compositionString.Length, kIMECharBufferSize);

might be clearer.

fixed(char* dst = compositionString)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we have both compositionString member and parameter to function isn't one of the src or dst pointing to the wrong one. I think we need dst = buffer

fixed(char* src = compositionString)
UnsafeUtility.MemCpy(dst, src, inputEvent.length * sizeof(char));
return inputEvent;
}
}

/// <summary>
/// A specialized event that contains the current IME Composition string, if IME is enabled and active.
/// This event contains the entire current string to date, and once a new composition is submitted will send a blank string event.
/// </summary>
[StructLayout(LayoutKind.Explicit, Size = InputEvent.kBaseEventSize + sizeof(int))]
public struct IMECompositionEventVariableSize : IInputEventTypeInfo
{
// Before we had 0x494D4553 which corresponds to ImeCompositionStringInputEventData fixed size event with 64 character payload.
// 0x494D4543 corresponds to ImeCompositionInputEventData and is a different event which provides variable size array of characters after the event.
public const int Type = 0x494D4543;

[FieldOffset(0)]
public InputEvent baseEvent;

[FieldOffset(InputEvent.kBaseEventSize)]
internal int length;

internal static unsafe char* GetCharsPtr(IMECompositionEventVariableSize* ev)
{
return (char*)((byte*)ev + InputEvent.kBaseEventSize + sizeof(int));
}

public FourCC typeStatic => Type;

/// <summary>
/// Returns composition string for the given event.
/// </summary>
/// <param name="ev">Pointer to the event.</param>
/// <returns>Composition string.</returns>
public static unsafe IMECompositionString GetIMECompositionString(IMECompositionEventVariableSize* ev)
{
return new IMECompositionString(GetCharsPtr(ev), ev->length);
}

/// <summary>
/// Queues up an IME Composition Event. IME Event sizes are variable, and this simplifies the process of aligning up the Input Event information and actual IME composition string.
/// </summary>
/// <param name="deviceId">ID of the device (see <see cref="InputDevice.deviceId"/>) to which the composition event should be sent to. Should be an <see cref="ITextInputReceiver"/> device. Will trigger <see cref="ITextInputReceiver.OnIMECompositionChanged"/> call when processed.</param>
/// <param name="str">The IME characters to be sent. This can be any length, or left blank to represent a resetting of the IME dialog.</param>
/// <param name="time">The time in seconds, the event was generated at. This uses the same timeline as <see cref="Time.realtimeSinceStartup"/></param>
public static unsafe void QueueEvent(int deviceId, string str, double time)
{
var sizeInBytes = (InputEvent.kBaseEventSize + sizeof(int)) + sizeof(char) * str.Length;
var eventBuffer = new NativeArray<byte>(sizeInBytes, Allocator.Temp, NativeArrayOptions.UninitializedMemory);

var ev = (IMECompositionEventVariableSize*)eventBuffer.GetUnsafePtr();

ev->baseEvent = new InputEvent(Type, sizeInBytes, deviceId, time);
ev->length = str.Length;

if (str.Length > 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this need braces around the next 2 lines ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't need braces the same way as:

if(...)
  if(...)
    doSomething();

doesn't need braces either.

But maybe I can add them to make code formatter happy.

fixed(char* p = str)
UnsafeUtility.MemCpy(GetCharsPtr(ev), p, str.Length * sizeof(char));

InputSystem.QueueEvent(new InputEventPtr((InputEvent*)ev));

eventBuffer.Dispose();
Copy link
Collaborator

Choose a reason for hiding this comment

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

After a bit of digging to understand InputSystem.QueueEvent it seems it makes a copy of the input event and therefore this dispose is valid. No feedback here. Just noting for my own reminder.

}
}

/// <summary>
/// A struct representing an string of characters generated by an IME for text input.
/// </summary>
Expand All @@ -42,36 +121,31 @@ public static IMECompositionEvent Create(int deviceId, string compositionString,
/// <see cref="ITextInputReceiver.OnIMECompositionChanged"/> method. It can easily be converted to a normal C# string using
/// <see cref="ToString"/>, but is exposed as the raw struct to avoid allocating memory by default.
/// </remarks>
[StructLayout(LayoutKind.Explicit, Size = sizeof(int) + sizeof(char) * LowLevel.IMECompositionEvent.kIMECharBufferSize)]
public unsafe struct IMECompositionString : IEnumerable<char>
{
internal struct Enumerator : IEnumerator<char>
private const int kLegacyIMEEventCharBufferSize = 64;

private readonly string m_ManagedString;
private readonly int m_Size;
private fixed char m_FixedBuffer[kLegacyIMEEventCharBufferSize];

private struct FixedBufferEnumerator : IEnumerator<char>
{
IMECompositionString m_CompositionString;
char m_CurrentCharacter;
int m_CurrentIndex;
private IMECompositionString m_CompositionString;
private int m_CurrentIndex;

public Enumerator(IMECompositionString compositionString)
public FixedBufferEnumerator(IMECompositionString compositionString)
{
m_CompositionString = compositionString;
m_CurrentCharacter = '\0';
m_CurrentIndex = -1;
}

public bool MoveNext()
{
int size = m_CompositionString.Count;

m_CurrentIndex++;

if (m_CurrentIndex == size)
if (m_CurrentIndex + 1 >= m_CompositionString.Count)
return false;

fixed(char* ptr = m_CompositionString.buffer)
{
m_CurrentCharacter = *(ptr + m_CurrentIndex);
}

m_CurrentIndex++;
return true;
}

Expand All @@ -84,58 +158,73 @@ public void Dispose()
{
}

public char Current => m_CurrentCharacter;
public char Current => m_CompositionString[m_CurrentIndex];

object IEnumerator.Current => Current;
}

public int Count => size;
public int Count => m_Size;

public char this[int index]
{
get
{
if (m_ManagedString != null)
return m_ManagedString[index];

if (index >= Count || index < 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this go before the m_ManagedString line above for safety?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't matter that much, in this case [ ] operator of the string will do the check anyway.

throw new ArgumentOutOfRangeException(nameof(index));

fixed(char* ptr = buffer)
{
return *(ptr + index);
}
return m_FixedBuffer[index];
}
}

[FieldOffset(0)]
int size;

[FieldOffset(sizeof(int))]
fixed char buffer[IMECompositionEvent.kIMECharBufferSize];

public IMECompositionString(string characters)
public IMECompositionString(char* characters, int length)
{
if (string.IsNullOrEmpty(characters))
// only allocate string if we can't fit into fixed buffer
if (length <= kLegacyIMEEventCharBufferSize)
{
m_ManagedString = null;
m_Size = length;
if (m_Size > 0)
{
Debug.Assert(characters != null);
fixed(char* dst = m_FixedBuffer)
UnsafeUtility.MemCpy(dst, characters, m_Size * sizeof(char));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering here if this is a call to native code and if spinning a loop and staying in managed is faster.

}
}
else
{
size = 0;
return;
m_ManagedString = new string(characters, 0, length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating a string like this, with a pointer to memory that we don't own, seems like a bad ideaTM. In the happy path through InputManager, that memory points to the native memory for the event buffer, which is going to be overwritten in the next frame. If the user keeps hold of the IMECompositionString outside the event handler, that's probably a crash. So I guess we need to make a copy here. Maybe use native memory, implement IDisposable on the struct, and add leak detection in the editor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that constructor would make a copy of the buffer, but I could be wrong.

m_Size = length;
}
}

Debug.Assert(characters.Length < IMECompositionEvent.kIMECharBufferSize);
size = characters.Length;
for (var i = 0; i < size; i++)
buffer[i] = characters[i];
public IMECompositionString(string characters)
{
// string is already allocated on the heap, so reuse it
m_ManagedString = characters;
m_Size = characters.Length;
}

public override string ToString()
{
fixed(char* ptr = buffer)
{
return new string(ptr, 0, size);
}
if (m_Size == 0)
return string.Empty;

if (m_ManagedString != null)
return m_ManagedString;

fixed(char* ptr = m_FixedBuffer)
return new string(ptr, 0, m_Size);
}

public IEnumerator<char> GetEnumerator()
{
return new Enumerator(this);
if (m_ManagedString != null)
return m_ManagedString.GetEnumerator();

return new FixedBufferEnumerator(this);
}

IEnumerator IEnumerable.GetEnumerator()
Expand Down
6 changes: 3 additions & 3 deletions Packages/com.unity.inputsystem/InputSystem/InputManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3252,11 +3252,11 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
break;
}

case IMECompositionEvent.Type:
case IMECompositionEventVariableSize.Type:
{
var imeEventPtr = (IMECompositionEvent*)currentEventReadPtr;
var imeEventPtr = (IMECompositionEventVariableSize*)currentEventReadPtr;
var textInputReceiver = device as ITextInputReceiver;
textInputReceiver?.OnIMECompositionChanged(imeEventPtr->compositionString);
textInputReceiver?.OnIMECompositionChanged(IMECompositionEventVariableSize.GetIMECompositionString(imeEventPtr));
break;
}

Expand Down