NBTTooltip icon indicating copy to clipboard operation
NBTTooltip copied to clipboard

Fixed bug: keybindings bound to non keyboard keys caused GL ERROR every tick

Open Klotzi111 opened this issue 3 years ago • 7 comments

Now using KTIG for keybindings (My new lib: https://github.com/Klotzi111/ktig) fixed bug: auto scroll will win against manual scroll when having very low framerate Scrolling the NBTTag now also works with mouse scroll when using amecs-api v1.2.1

Klotzi111 avatar Sep 21 '21 13:09 Klotzi111

Thank you for your contribution, but I'd rather not depend on anything. This is a small utility targeted towards mod and pack developers.

Plus I don't have the time to check the library to make sure it's trustworthy and stable for me to require as a dependency.

There must be a way to use non standard keys in fabric api alone, otherwise that's major enough to warrant a direct fix on the api itself.

I'll look into the low framerate making autoscroll win over the lock, I wasn't aware of the issue, but I can think of a few ways to fix it without introducing dependencies.

zabi94 avatar Sep 21 '21 14:09 zabi94

The auto scroll winning problem is very easily fixed without the library. Just look at my changed code.

I understand that you do not want to add a dependency/library. But the keybinding + gui problem is in almost every mod and my mission is to fix this major problem in a central library. So if many mods use this lib in the future you can easily use the lib as well.

I will try to bring the lib to fabric api but thats probably not going to happen because they do not add new stuff.

Klotzi111 avatar Sep 21 '21 15:09 Klotzi111

This is more of a bugfix than new functionality. Minecraft allows non standard keybinds, fabric doesn't handle them correctly

zabi94 avatar Sep 21 '21 15:09 zabi94

Minecraft allows non standard keybinds

Kind of. You can add new keycodes/mousebuttons to KEYSYM, SCANCODE or MOUSE.

fabric doesn't handle them correctly

Thats totally wrong. Fabric API has nothing to do with (non-)/standard keys. The bug comes from you code: https://github.com/zabi94/NBTTooltip/blob/7c3e38adbf8d5b2e018202de6a6c50f37ddca3e2/src/main/java/zabi/minecraft/nbttooltip/NBTTooltip.java#L114 You use InputUtil.isKeyPressed and that is ONLY for keyboard keycodes. To support at least (almost) all vanilla keys (mouse and keyboard) you would need to change the method to:

	private static boolean isPressed(MinecraftClient mc, KeyBinding key) {
		if(key.isUnbound()) {
			return false;
		}
		Key keyObject = InputUtil.fromTranslationKey(key.getBoundKeyTranslationKey());
		long window = mc.getWindow().getHandle();
		if(keyObject.getCategory() == InputUtil.Type.SCANCODE) {
			return InputUtil.isKeyPressed(window, keyObject.getCode());
		} else if (keyObject.getCategory() == InputUtil.Type.MOUSE) {
			return GLFW.glfwGetMouseButton(window, keyObject.getCode()) == 1;
		}
		return false;
	}

That would be way better but still not perfect

Klotzi111 avatar Sep 21 '21 15:09 Klotzi111

Guess I'll fix it then. I kinda assumed they weren't in different categories

zabi94 avatar Sep 21 '21 15:09 zabi94

I just realized there is absolutely 0 abstraction done on keys. On my linux system almost every key has type KEYSYM, that snippet you posted wouldn't work at all for me. The one button I can get to be scancode is a device I have, that results in a button called scancode.191. InputUtil.isKeyPress doesn't work with that, I need to use the wasPressed() method, which doesn't work when a screen is open apparently, which is where most of my keys would be pressed.

That's rough.

zabi94 avatar Oct 02 '21 10:10 zabi94

Yea exactly that is why I made KTIG and used in this pr to fix ALL those problems. I even told you that my snippet is no 100% solution. But my lib (KTIG) is (Or at least it should be and if it happens to be that it is not I will make it work). But if you do not want to use it I can not help.

Klotzi111 avatar Oct 02 '21 13:10 Klotzi111