Gamepad axis inputs causing crashes

[hissingshark]

Further to #457 I've got the client built and running in full-screen/non-X11.

My final issue is with gamepad support. Any axis events (including dpad which I know are also treated as full deflection axes) result in a crash. This might be immediate or take a few minutes to occur. They may work in the game menu, but then the crash will occur whilst the game world is loading. I've seen 4 different errors:

free(): invalid next size (fast)

malloc(): invalid next size (unsorted)

corrupted double-linked list

free(): invalid next size (fast)
corrupted size vs. prev_size

The callbacks I'm using are:
onGamepadState to indicate the controller is connected,
onGamepadButton for the button presses, and
onGamepadAxis for the axes. Of course - the axis values I supply are normalised to a -1 to 1 range.

As a further datapoint - the dpad axes behave strangely in the game menu. No matter if I press up or down, the menu moves down and it keeps going as if I'm holding the button. I've added debug lines and it is definitely receiving the press and release events.
The same thing happens with left/right, except it only goes endlessly to the right.

---

FYI:
I'm using straight up EGL for video. I abandoned SDL as there were transparency issues with some 2D elements that I never managed to resolve. Also SDL cannot supply a mouse pointer when it isn't windowed. So I'm rendering my own "diamond sword" mouse pointer to the framebuffer.

I'm still using SDL for input. Keyboard and mouse are working fine and the game is playable.

---

As ever, any suggestions are greatly appreciated.

[ChristopherHX]

free(): invalid next size (fast)
malloc(): invalid next size (unsorted)
corrupted double-linked list
free(): invalid next size (fast)
corrupted size vs. prev_size

I saw all these errors on amd64 too :/, the previous gamepad input api (removed in ng, by mrarm) of the Launcher worked far better.
Side note I reduced the error rate for my x86_64 appimage via minecraft-linux/mcpelauncher-client@d797452, but it is still very instable.

My idea would be to cherry pick the old direct interface code back (https://github.com/ChristopherHX/mcpelauncher-client/blob/master/src/window_callbacks.cpp#L144-L181). This would also allow splitscreen with 1.14.1.5 or earlier again.

the dpad axes behave strangely in the game menu

Known arm32 only bug (arm64 works fine), but shouldn't be hard to fix...., did that already in libc-shim.
Dpad's and axes are passed as floating point values, which leads to a soft float (Android) versus hardfloat (armhf) glitch.
Wrapping some methods via https://github.com/minecraft-linux/libc-shim/blob/master/src/armhfrewrite.h ARMHFREWRITE(method) can fix the calling convention mismatch.
At least the following lambda needs to be wrapped with ARMHFREWRITE
https://github.com/minecraft-linux/mcpelauncher-client/blob/9d5f9bddce0e6add5a5d7dc3472c85990a6790f0/src/fake_inputqueue.cpp#L57 , in order to avoid the calling convention missmatch

I'm still using SDL for input

Please contribute your SDL implementation (as a Pull Request) to https://github.com/minecraft-linux/game-window even if it is incomplete.

[hissingshark]

Thanks for the feedback.
I can't imagine what it was like before you reduced the error rate with d797452. With a gamepad it's taking me so many attempts to even get into the game as it is!

---

My backup plan was to provide gamepad support by translating the inputs into mouse and keyboard callbacks. But I'd rather take the time to do this properly. I'll start reading your old code to try and understand what I need to implement.
Why was the "old direct interface code" removed if it will fix this? Does it cause issues for other platforms?

---

I've tried many times over the past few days to wrap the lambda, but but without success. Honestly I'd never heard of lambda functions until last month when I started picking apart WindowCallbacks::queueGamepadAxisInputIfNeeded to debug the crashes!
In the end I moved the lambda to an actual function so I could wrap that instead:

float getAxisValue(const AInputEvent *event, int32_t axis, size_t pointerIndex) {
    return ((const FakeMotionEvent *) (const void *) event)->axisFunction(axis);
};

and then:
syms["AMotionEvent_getAxisValue"] = ARMHFREWRITE(getAxisValue);
It's not how you intended I'm sure, but it does prove that it fixes the input bug and I can now use the analog sticks in game too. For my platform this would require an #ifdef I guess to make it official, with a separate PR to https://github.com/minecraft-linux/mcpelauncher-client

---

I didn't know if you'd be interested in a PR of my SDL code, but it was always to the plan to submit one for changes/suggestions when it was finished. So of course, very happy to contribute to the project.

[hissingshark]

@ChristopherHX
OK, I think I follow your direct interface code. The implementation we can't see because it's actually the game binary itself? So you've generated a header and symbols for interfacing with it at runtime. From what I've found, to access methods like GameControllerManager::sGamePadManager->setGameControllerConnected as you did, I'd need to reintroduce:
this header file
and these symbols
from the old minecraft-symbols, which I guess was replaced in ng by minecraft-imported-symbols.

[ChristopherHX]

So you've generated a header and symbols for interfacing with it at runtime

Not my code, I haven't written it. Yes the implementation is in libminecraftpe.so as long they keep exporting these symbols, this avoids using the android input interface.

from the old minecraft-symbols, which I guess was replaced in ng by minecraft-imported-symbols.

No was removed (not replaced, minecraft-imported-symbols was kept) by mrarm, he removed more code than I in my fork.
I have technically an unpublished version of minecraft-symbols for ng, maybe I will find it somewhere.

Moreover on my machine x86_64 I cannot reproduce the

free(): invalid next size (fast)

malloc(): invalid next size (unsorted)

corrupted double-linked list

free(): invalid next size (fast)
corrupted size vs. prev_size

errors anymore, my machine just keep running no matter what I do with my gamepad. This is also true for the random crashes.
I even enabled a memory leak sanitizer, but that found nothing on my machine. weird.

[ChristopherHX]

I forget I published it...

• Add https://github.com/ChristopherHX/minecraft-symbols as a submodule
• You have to regenerate it for armhf with a python script (my fork has a readme about this), (well this may revert some parts of my last commit and won't compile on armhf, fixed with ChristopherHX/minecraft-symbols@8d1d7e8)
• Add it to cmake
• call minecraft_symbols_init(handle); in main after loadMinecraft

[hissingshark]

Good stuff.
I've cloned that into the manifest and run the python script.
I added add_subdirectory(minecraft-symbols) to mcpelauncher-manifest/CMakeLists.txt
In mcpelauncher-client/src/main.cpp I used #include <minecraft/symbols.h> and the minecraft_symbols_init(handle); line.

First sign there was a problem with cmake was:

fatal error: 'minecraft/symbols.h' file not found

So I changed the include temporarily to an absolute #include "../../minecraft-symbols/include/minecraft/symbols.h"
That yielded a linking error:

/usr/bin/ld: CMakeFiles/mcpelauncher-client.dir/src/main.cpp.o: in function main': main.cpp:(.text+0x1db4): undefined reference to minecraft_symbols_init(void*)'

My build directory contains the minecraft-symbols output:

minecraft-symbols/
├── CMakeFiles
│ ├── CMakeDirectoryInformation.cmake
│ ├── minecraft-symbols.dir
│ │ ├── build.make
│ │ ├── cmake_clean.cmake
│ │ ├── cmake_clean_target.cmake
│ │ ├── CXX.includecache
│ │ ├── DependInfo.cmake
│ │ ├── depend.internal
│ │ ├── depend.make
│ │ ├── flags.make
│ │ ├── include
│ │ │ └── minecraft
│ │ │ └── std
│ │ │ └── string_linux.cpp.o
│ │ ├── link.txt
│ │ ├── progress.make
│ │ └── src
│ │ ├── symbols.cpp.o
│ │ └── symbols_dlsym.cpp.o
│ └── progress.marks
├── cmake_install.cmake
├── libminecraft-symbols.a
└── Makefile

What else would you have done in CMakeLists.txt? I can't see that you treated any of the other submodules differently.

Many Thanks.

[ChristopherHX]

You aren't a cmake expert, but I have had such problems too in the past :).You need to add minecraft-symbols, in the target_link_libraries of mcpelauncher-client cmake file. This fixes both the header file not found and linker errors. (some links https://cmake.org/cmake/help/latest/command/target_link_libraries.html, https://github.com/ChristopherHX/minecraft-symbols/blob/8d1d7e880ac453ab4fa5d741cc78b773784557e6/CMakeLists.txt#L13, https://github.com/minecraft-linux/mcpelauncher-client/blob/d68cf09de902f683783075b7bb59f818c3278c8e/CMakeLists.txt#L12).In mcpelauncher-client/CMakeLists.txt change 'target_link_libraries(mcpelauncher-client logger mcpelauncher-core gamewindow filepicker msa-daemon-client cll-telemetry argparser baron android-support-headers libc-shim OpenSSL::Crypto ${CURL_LIBRARIES})' to 'target_link_libraries(mcpelauncher-client logger mcpelauncher-core gamewindow filepicker msa-daemon-client cll-telemetry argparser baron android-support-headers libc-shim minecraft-symbols OpenSSL::Crypto ${CURL_LIBRARIES})'

[hissingshark]

Ah, of course. It's the client that must import, not the manifest...
Yes you're right. Most of what I know of cmake has been from reverse engineering the CMakeLists of this project and a few others like PPSSPP, then reading the docs. Always learning!

So with that done I just needed to re-add an old header file, delete a few duplicate symbol definitions and re-write the callback functions to use the modern gamepad data structure/array. All is working now, thank you.
I'll tidy up and submit a PR for my SDL window.

---

Regarding these changes reintroducing part of the old interface, is that worth submitting a PR for too, as a cmake option perhaps? Alternatively, would you like me to do anything more to help debug the underlying issue?

[hissingshark]

I was nearly there! I realised I'd forgotten to implement mousewheel. Having done so I found that only up worked. I encountered 2 issues.

1. char is unsigned in my system, so I had to change the callback to:

void WindowCallbacks::onMouseScroll(double x, double y, double dx, double dy) {
signed char cdy = (signed char) std::max(std::min(dy * 127.0, 127.0), -127.0);

Otherwise down evaluates to 0 instead of -127.

2. Having fixed that, up and down both scroll up! I take it this is the armhf issue again? But I just can't work out how to wrap Mouse::feed with ARMHFREWRITE. I see where it is loaded in symbols.cpp, but it's the same issue as before. We have a function pointer, but ARMHFREWRITE wants "&func" (which I thought was essentially what a function pointer is). Do we need to recast the pointer into a function or something?

Many thanks.

[ChristopherHX]

I'll tidy up and submit a PR for my SDL window.

I'am looking forward to see it :), I wonder if it allows me to build it for android to compare bugs due to libc-shim.

1. char is unsigned in my system, so I had to change the callback to:

I didn't pulished a fix, because I know it won't work, because of point 2.

ARMHFREWRITE, is useless for Mouse::feed, because char is not a floatingpoint function, I tried it already.
But there might be hope, 1.16.230 (beta) officially supports mouse on android so there might be a new arm compatible way to trigger scrolling of the game, sometime in the future. I never debugged this in gdb assembly so idk what to do here.

Regarding these changes reintroducing part of the old interface, is that worth submitting a PR for too, as a cmake option perhaps?

A cmake options for this would be a good start (via cli on runtime would be even better or?)
I would merge it if it looks good for me. I wondered why it was removed in first place.

[hissingshark]

I'm closing this now then, as we've documented the workarounds here and I have a working input system.