User Input Events v1

[yyny]

I need something like this for 1.16.4

[i509VCB]

This is going to need some... cleanup.

I will comment around that things that probably need to be cleaned up

[i509VCB]

Doesn't every mixin expose inject their symbols into the target class? I'm don't think it's needed here, but what about the @ModifyVariables?

At runtime mixin partially scrambles the names of injectors/redirects/modifywhatever. - so onBlockBreak -> inject$zzz000$onBlockBreak.

Mixin does not scramble if the thing being implemented is a duck interface

[i509VCB]

Also some advice: You can run the gradlew check task and you will get a list of style issues printed. gradlew licenseFormat will apply all license headers for you.

[yyny]

You can run the gradlew check task and you will get a list of style issues printed. gradlew licenseFormat will apply all license headers for you.

Thanks. Put it in a CONTRIBUTING.md!

[i509VCB]

We do have most of a contribution doc but I need to publish it here

[i509VCB]

Okay so the big changes start here. Typically we would just pass the parameters here, such as key code, modifiers, etc

This avoids the need to needlessly allocate an object here.

[yyny]

I might start an issue about this because I think this is the wrong way to go about things.

In this case, specifically, it is very useful to obtain additional information not originally provided by the event, such as getting the mouse position or buttons being held during a scroll event or KeyEvent::getKey and KeyEvent::isPressed.

But, I think it is good to limit the listeners to one input even in the general case because then events would map much more cleanly to monads/the observer pattern (without wrapper overhead).

Adhering to the observer pattern would also make the design of the future event generation codegen much easier if any would even be needed at all (Observers compose really well).

This avoids the need to needlessly allocate an object here.

You can cache objects and use getter interfaces to prevent mutability. One function call of extra overhead but it has no allocation cost and makes the entire events API much more extensible (can add new inputs at any time). I don't think the overhead is even an issue for 99% of use cases (Can just call the function once and store the result in a local variable, everyone does that for Minecraft.getInstance already anways), and in the rare case we do need to call thousands of listeners per second we still have the option of providing "unsafe" events that provide the mutable event object as input (Under the contract that no listeners modify the input, of course). It's probably even possible to create a class with final members and then use ASM to modify the cached event object.

All that together would actually be faster because it doesn't have the same hidden cost of allocation stack frames and copying parameters over and over again.

[yyny]

Actually, on the topic of mutability, aren't we already implicitly requiring listeners to not modify their inputs anyway?

[i509VCB]

Not going to be able to comment much more for a few hours - hopefully stuff so far will bring the pr up to step

[yyny]

I will clean up some more later tonight

[liach]

You should move this to some sort of cached supplier, which you can then expose to your event callbacks, instead of adding a hasListeners in the events.

[liach]

Lazy<KeyBinding> lazyBinding = new Lazy<>(() -> {
    Map<InputUtil.Key, KeyBinding> keyToBindings = KeyBindingAccessor.getKeyToBindings();
    return keyToBindings.get(InputUtil.fromKeyCode(code, scancode));
});

Hmm, I now wonder if you should rather pass in like a functional interface with a method KeyBinding getKey(int scancode, int keycode) for performance reasons. It's debatable, but not of priority.

[yyny]

This seems to add a lot of complexity and inconvenience, and an extra object allocation every key-press which I was told wasn't allowed, for very little benefit. If we are going to allocate a functional interface anyway, I would much rather go back to the original KeyEvent implementation, considering all the benefits I have outlined for that approach earlier.

[liach]

I don't know how this works out with the concurrency-compatible design of fab events (that's why invokers are a field because player wants you to update it real time than through some delay). But such logic usually have problems in a concurrent setting, where you are supposed to just try do an action and see its results than ask if you can do and then do an action (during the ask and do something else can meddle)

[i509VCB]

If I recall player does do COW for adding new listeners in the events.

[liach]

Yes, I believe he does that though generating new invokers through the function supplied to createArrayBacked, which is an array to created invoker function. In an invocation the old invoker can be removed from the event instance but its invocation will continue, which is effectively copy-on-write.

[yyny]

I don't see this as an issue right now because ArrayBackedEvent already has a race condition anyway, the hasListeners only makes the race window slightly bigger, might need to be documented but not sure about refactoring. Notice the small window between updating the handlers and changing the invoker. If an Event implementation really wants to be as concurrent as possible it doesn't even have to implement this function anyway. This approach can only ever extend the window if an invoker is added in between the call to Event#hasListeners() and the call to Event#invoker(). As with the dummyInvoker optimization in ArrayBackedEvent or the COW, there really is no way to do this optimization without creating a race condition. Do we want to optimize away ~100 bytecode instructions for every event invocation (If no-one is listening), or do we want event listeners to start being able to receive events ~100 bytecodes earlier? It doesn't seem like a big deal to me to not invoke listeners for the sake of optimization for events that were emitted before the listener was even added.

Another option I considered was to instead expose a T Event#noop() that the invoker can be compared against (Would return this.dummyHandler in ArrayBackedEvent), this doesn't change the race condition in any way, but it might make it more obvious that the invoker() is slightly out of date by the time it gets invoked. I preferred the boolean approach purely for performance reasons because it doesn't force the event implementation to cache a noop handler if it wants to have this optimization.

[yyny]

I also noticed another big flaw.

Most of the events never check if the window they're triggered for is the same as the MC's window, which destroys the possibility for mod authors to create their own windows (even if really niche, this has to be considered).

This doesn't seem like a big flaw, if the mod doesn't take care to mix into the input event handlers, then the Vanilla code would be executed as well, since Minecraft doesn't check this condition either. And I've mentioned this before, but if we have an input event, it would be more obvious for mod developers that want to do crazy things like that to figure out how they should go about implementing such a feature without breaking any mods.

[LemmaEOF]

None of the changes made have fixed the fundamental problem: this way of doing things is antithetical to the Fabric ethos. This PR will still break dozens of mods and add an unacceptable amount of runtime bloat. I'm exercising my authority as a member of the triage team to close this PR. Put this in your own library; it's literally free.

[i509VCB]

Reopening this since the implementation has changed and actually uses synthetic method injects which is more mod compatible and does not use the extra event object anymore as seen in e11c4b8 and beyond, just some areas still need to be ironed out such as some possibly redundant methods being dropped and documentation being used to refer to the builtin methods instead.

[liach]

@yyny

1. Thanks i509 for reopening. After another glance this is really fantastic
2. For your hasListeners, I am offering a different approach:

Now you can write this for your events: (please check the code, I wrote it by hand on github and this may have compile errors)

	public static final Event<KeyState> KEY_REPEATED = EventFactory.createArrayBacked(KeyState.class, listeners -> (code, scancode, modKeys, nullKey) -> {
		// no listeners? no object creation! but still need to check keybind version
		if (listeners.length == 0) {
			// change the event name to the corresponding keybind version
			KEYBIND_REPEATED.invoker().onKeybind(code, scancode, modKeys, null, null); // let them compute both if they need to
			return;
		}
		
		Key key = InputUtil.fromKeyCode(code, scancode);
		
		for (KeyState listener : listeners) {
			listener.onKey(code, scancode, modKeys, key); // listeners are guaranteed to receive non-null keys
		}
		// same, change event name
		KEYBIND_REPEATED.invoker().onKeybind(code, scancode, modKeys, key, null); // let them compute keybind on need
	});

	// Keybind version
	public static final Event<KeybindState> KEYBIND_REPEATED = EventFactory.createArrayBacked(KeybindState.class, listeners -> (code, scancode, modKeys, nullableKey, nullKeybind) -> {
		// no extra object creation
		if (listeners.length == 0) {
			return;
		}
		
		// So when we pass key and key bindings to invoker() we pass null; we compute it here
		Key key = nullableKey == null ? InputUtil.fromKeyCode(code, scancode) : nullableKey; // compute if absent
		KeyBinding binding = KeyBindingAccessor.getKeyToBindings().get(key); // compute it here
		
		for (KeybindState listener : listeners) {
			listener.onKeybind(code, scancode, modKeys, key, binding); // listeners are guaranteed to receive non-null keys and bindings
		}
	});

And replace this huge chunk of code at https://github.com/FabricMC/fabric/pull/1272/files#diff-2ee30e5d1b4f872827ccff6490183560cffe52bc0b983ffc0005b0b5c08c5698R39-R76 with:

	// So when we pass key objects to invoker() we pass null; we compute it on demand
	switch (action) {
	case GLFW.GLFW_PRESS:
		ClientInputEvents.KEY_PRESSED.invoker().onKey(code, scancode, modKeys, null);
		break;
	case GLFW.GLFW_RELEASE:
		ClientInputEvents.KEY_RELEASED.invoker().onKey(code, scancode, modKeys, null);
		break;
	case GLFW.GLFW_REPEAT:
		ClientInputEvents.KEY_REPEATED.invoker().onKey(code, scancode, modKeys, null);
		break;
	}

Thus, hasListeners is rendered useless and can be safely removed.

Wait a sec, I am not that pleased. Please let me open ide and give you a better version

[liach]

Just fyi I made yyny#1 which includes my suggestions. Now I think my suggestions are still quite cursed.

[liach]

@Technici4n Should this target 1.18 or even 1.19 now, or will we merge into 1.16 first and then port?

[Technici4n]

I'd say target 1.19 as it will take a long time to properly review (quite low priority).

[Phoupraw]

Mark and top.