User Input Events v1

[yyny]

I need something like this for 1.16.4

[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]

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

[yyny]

I honestly don't really understand the point of these events as usually all input related queries are done in tick methods or in Screen

Because I don't need to do the usual querying or screen handling, I need clientside events. And I don't need keybinds, I need raw key presses. See also #129 and Forge InputEvent.KeyInputEvent. I am not alone in this.

[yyny]

What is your use case for this

It's a port for a scripting API. Scripts listen for keyboard events to implement minigames or special items without having to write java.

[yyny]

its a huge amount to maintain and support for very little gain

I mean its ~1000loc including comments so I don't know about that first part

I have a feeling this is out of scope for fabric, for listening for key presses we have a keybinding API, events like this cover a tiny amount of cases were a keybind wont work.

This seems like at least one reason why having a single unifying event helps portability.

[yyny]

Please inject into synthetic/lambda methods of Keyboard instead.

I am working on this, btw, but sponge is causing my gradle builds to crash.

[LemmaEOF]

This seems like at least one reason why having a single unifying event helps portability.

you're literally citing someone saying that your PR will break things and make them less portable...

[yyny]

you're literally citing someone saying that your PR will break things and make them less portable...

Yes, in it's current state.

[yyny]

I think that the fact that the naive implementation I wrote here already has portability issues shows that there is demand for a more complete unifying event. If not in fabric, then in a library or another project like architectury.

[yyny]

I have a feeling this is out of scope for fabric

I'd like to clarify for just a bit, I didn't originally submit this pull request because I think fabric absolutely needs it, I'm submitting it because I need it, because issue #129 from 2019 hasn't neither been closed nor even been commented on yet, and because I just wanted to get this part of my mod done instead of waiting another year. I already wrote most of the original code before I even considered submitting it. From my POV, this is the right place to put this code, because:

1. I am porting a large legacy mod that I am intending to be compatible and portable for a long time to come
2. Solving portability issues that have nothing to do with my code directly shouldn't be a concern for me, but for fabric itself.
3. Solving portability issues together in a single repo seems much more efficient than on an individual mod-by-mod basis, especially for fundamental systems like user input (Case point: Within days of me submitting the pull request someone immediately reported an issue).
4. Solving portability issues seems to be the entire reason why fabric-api exists in the first place.

   Fabric API is the library for essential hooks and interoperability mechanisms for Fabric mods. Examples include: Adding events, hooks, and APIs to improve interoperability between mods.

5. Forge has had these events too without issues
6. There was interest in events like this all the way in 2019, so clearly there is a demand for it in Fabric too.
7. The events themselves are rather trivial, it's the implementation that is the only issue here, I'd rather argue about that than about its existence in fabric because, at least from my POV, it seems obvious.

Preferably, we would want to support @LambdAurora's use-case as well, but once again, maintenance for that has to be done collaboratively. If it wasn't for this pull request to bring other people in on it, I wouldn't have even known my mod has compatibility issues.

Also:

for listening for key presses we have a keybinding API

I'm pretty sure the API does not actually handle that

[LambdAurora]

for listening for key presses we have a keybinding API

I'm pretty sure the API does not actually handle that

It kind of does, the API allows you to register keybindings and the "normal" way of listening for key presses with those is calling KeyBinding#wasPressed in a tick method.

Though if a KeyBinding press event was done it would need to take into consideration mods that allows keybindings to be pressed using multiple keys, which becomes quite complicated very quickly.

[yyny]

It kind of does, the API allows you to register keybindings and the "normal" way of listening for key presses with those is calling KeyBinding#wasPressed in a tick method.

Though if a KeyBinding press event was done it would need to take into consideration mods that allows keybindings to be pressed using multiple keys, which becomes quite complicated very quickly.

So then the API only handles registering keybindings then, once again what I need is an event. It would be inefficient to check key bindings on ticks where no new input key was pressed. Also, I need to check all keybindings, not just specific ones (Well, technically, for right now I only need the Vanilla keybindings but I can see exposing modded keybinds at some point aswell). If other mods want to do things like pretend a key is being pressed or allow for multi-key keybinds, then that just seems like one more reason for me to add input events to fabric-api.

You do make a good point that perhaps these events should only be emitted once per tick, although I see no real harm with the current implementation regarding that. Also, I'll have to test if the current implementation works for custom keybindings as you mentioned.

[yyny]

Apparently, Forge only sends InputEvents when there isn't a screen being opened. I think the event system implemented here is more versatile, but it's at least something to consider.

If all Events were Observables, then we could simply (and efficiently) do InputEvents.KEY_PRESSED_FORGE = InputEvents.KEY_PRESSED.filter(key -> MinecraftClient.getInstance().currentScreen == null);.

[yyny]

I don't see any real reason to expose any interfaces here as there is very little extra stuff you could be exposing here

This is always subjective, most events that get passed a MinecraftClient don't strictly need that either since they can always call MinecraftClient.getInstance().

In the case of KeyEvent, I chose to add Key key; in addition to the int code, int scancode, int action, int modKeys from the raw GLFW event because it already has to be looked up anyway for generating a KeybindEvent and because it seems useful enough to have. Same situation with KeybindEvent.key and MouseButtonEvent.key. All the others, like GenericKeyEvent#isPressed and MouseScrollEvent.cursorX are there purely for convenience, and because they are cheap to come by.

[yyny]

These input util calls imo can just be moved to api. The method bodies of api is effectively impl.

aight

[yyny]

I'm not entirely happy with FabricKeyboard and FabricMouse since they duplicate vanilla functionality, but I don't see a way to give the users the modifier keys any other way. I know from experience that modKeys don't always directly correlate to their corresponding key, for example some Linux window managers remap them, and some mice have special buttons for them, but I think people with setups like that know what they are getting into, so we might still just force users to do e.g. Keyboard.isPressed(GLFW.GLFW_KEY_CTRL) anyway. If users really need the modifier keys, we can keep them available in the raw input events (ClientInputEvents.KEY and ClientInputEvents.MOUSE_BUTTON). Would be code duplication if multiple API users need them, but oh well.

Same goes for the modKey and scancode parameters, we could consider removing those from the non-raw input events, Vanilla never uses them for anything, and I suspect most mods don't either. This would help reduce the number of arguments in lambda functions. Note that I do need at least the scancode for my particular use case, but this is the dilemma we get ourselves into when we only allow multi-argument lambda listeners. Either everyone has to pay the price of unused parameters, or someone has to pay the price of duplicated calculations (Or we can add 20 different events for every possible permutation of input arguments to fake composability, but yeah, let's not. I'd still argue record-like events (Immutable object sor getter interfaces) are the better solution here).

In it's current state, net.minecraft.client.Mouse.getX() and net.minecraft.client.Mouse.getY() are left outdated while we are in event listeners, in the same way it is outdated while inside a Screen handler. We can fix this ourselves, but that would break compatibility with vanilla (Although I suspect this is a bug in Minecraft). We could also inject after Vanilla has updated them, but that would limit the usability of these events, since then screen handlers will already have been called. Finally, we could deprecate getX and getY, although I don't know how harmful/useful they really are compared to FabricMouse. For now I will document the difference without making any additional changes.

[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 https://github.com/yyny/fabric/pull/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.