Skip to content
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

[NOSQUASH] TouchScreenGUI refactor #13640

Merged
merged 2 commits into from Jul 14, 2023

Conversation

srifqi
Copy link
Member

@srifqi srifqi commented Jul 2, 2023

Goal of the PR
This PR refactors TouchScreenGUI code to make it more safe and maintainable.

How does the PR work?
There are two parts of this PR (separated by two commits):

  1. Fixes memory leaks and code style issues.
  2. Simplifies calculations by using better data type.

Does it resolve any reported issue?
There are no reported issue on this, but I noticed that some members/properties/variables were not removed correctly after its parent object is destroyed. There are also some repeating parts inside the code, mainly for the virtual joystick.

Does this relate to a goal in the roadmap?
Yes, this PR tries to refactor internal code.

If not a bug fix, why is this PR needed? What usecases does it solve?
From my tests, there are indications of memory leaks when using TouchScreenGUI code.

To do

This PR is Ready for Review.

How to test

To test the change statically,

  1. Read the changes in the code base.
  2. Verify that the old code and the new code are the same functionally.

To test the change practically,

  1. Run Minetest on a touch-screen device.
  2. Play in a world or on a server.
  3. There should be no behaviour changes when using touch-screen GUI.
  4. Back to Menu.
  5. There should be no crashes.

To spesifically test memory leaks indication,

  1. Run Minetest (without this PR) on a touch-screen device.
  2. Play in a world or on a server.
  3. Wait until its RAM usage has settled.
  4. Note down its RAM usage.
  5. Back to Menu.
  6. Repeat step number 2–5 for a few times.
  7. Exit to OS.
  8. Run Minetest (with this PR) on a touch-screen device.
  9. Repeat step number 2–7 on this new version.
  10. Compare the results.

Android: APK builds

Older APK builds

@srifqi srifqi added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Code quality labels Jul 2, 2023
@srifqi
Copy link
Member Author

srifqi commented Jul 2, 2023

I tested this PR on Samsung Galaxy A72 (Android 13). I waited around 10 seconds in a world before sampling its RAM usage.

RAM usage in-game without exiting the app

Play Num. master (+) This PR (+)
1 224.0 MB - 230.6 MB -
2 229.3 MB +5.3 235.5 MB +4.9
3 234.2 MB +4.9 237.5 MB +2.0
4 238.0 MB +3.8 239.3 MB +1.8
5 240.5 MB +2.5 241.4 MB +2.1
6 244.4 MB +3.9 243.8 MB +2.4
7 248.3 MB +3.9 246.1 MB +2.3
Average +4.1 +2.6

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

The code looks good, the build passes and some tests were already done.
Minor comment, otherwise fine.

@SmallJoker
Copy link
Member

SmallJoker commented Jul 10, 2023

I got the following error with your v7a binary on Android 6 even after deleting /storage/emulated/0/Android/data/net.minetest.minetest manually and re-installing the app. I tested a self-signed binary from master which worked flawlessly, thus I assume there were a few bugfixes that are yet not included in your binary. Whereas this issue is pretty sure not caused by the PR itself, I think it should be mentioned either way - just in case.

2023-07-10 20:05:09: ERROR[Main]: Irrlicht: PNG fatal error: IHDR: CRC error
2023-07-10 20:05:09: WARNING[Main]: Could not load icon file.
2023-07-10 20:05:09: ERROR[Main]: Irrlicht: PNG fatal error: IHDR: CRC error
2023-07-10 20:05:09: ERROR[Main]: Irrlicht: Could not load texture: /storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest/textures/base/pack/checkbox_16.png
2023-07-10 20:05:11: ERROR[AsyncWorker-0]: HTTPFetch for servers.minetest.net/list?proto_version_min=37&proto_version_max=43 failed (Error)
2023-07-10 20:05:44: ERROR[Main]: Irrlicht: PNG fatal error: IHDR: CRC error
2023-07-10 20:05:44: ERROR[Main]: generateImage(): Could not load image "progress_bar.png" while building texture; Creating a dummy image
2023-07-10 20:05:44: ERROR[Main]: Irrlicht: PNG fatal error: IHDR: CRC error
2023-07-10 20:05:44: ERROR[Main]: generateImage(): Could not load image "progress_bar_bg.png" while building texture; Creating a dummy image
2023-07-10 20:05:44: ERROR[Main]: ModError: Failed to load and run script from /storage/emulated/0/Android/data/net.minetest.minetest/files/Minetest/games/minetest_game/mods/default/init.lua:
2023-07-10 20:05:44: ERROR[Main]: dcompressZlib: inflateInit failed
2023-07-10 20:05:44: ERROR[Main]: stack traceback:
2023-07-10 20:05:44: ERROR[Main]: 	[C]: in function 'orig_reg_fn'
2023-07-10 20:05:44: ERROR[Main]: 	...netest.minetest/files/Minetest/builtin/game/register.lua:445: in function 'register_decoration'
2023-07-10 20:05:44: ERROR[Main]: 	...les/Minetest/games/minetest_game/mods/default/mapgen.lua:1815: in function 'register_decorations'
2023-07-10 20:05:44: ERROR[Main]: 	...les/Minetest/games/minetest_game/mods/default/mapgen.lua:2491: in main chunk
2023-07-10 20:05:44: ERROR[Main]: 	[C]: in function 'dofile'
2023-07-10 20:05:44: ERROR[Main]: 	...files/Minetest/games/minetest_game/mods/default/init.lua:77: in main chunk
2023-07-10 20:05:44: ERROR[Main]: Für mehr Details siehe debug.txt.
2023-07-10 20:05:44: ACTION[Main]: Server: Shutting down

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Code LGTM

@srifqi
Copy link
Member Author

srifqi commented Jul 11, 2023

I have updated the APK builds (see the main post) with this PR after rebase and current master of the Minetest Game.

@SmallJoker
Copy link
Member

What I forgot to mention previously (and also applies to the newer build):

  • MTG does not show up in the game menu and there is no prompt to install a new game (outdated builtin?)
  • No icons can be loaded

I found that the directory /Android/data/net.minetest.minetest/files/Minetest/ is somehow cached by gvfs on Linux when mounting via MTP, thus I ended up looking at invalid directory contents without noticing. This is somewhat annoying, thus I have now opened an issue on libmtp in hopes to solve this.

Either way, the buildbot binary and yours both seem to install the same files. I double-checked the textures that failed to load but they show up in the file browser. Also a texture pack that I might have installed previously magically reappears even after deleting Minetest's data directory manually.

I cannot figure out what's wrong and since the build from master does work, I'd prefer to not dig into this any deeper.

Maximum line length is 95 characters.
Some members' name are changed.
Struct initialisations use brace syntax; eliminating the usage of the memset function.
Iterations use for-each-loop instead of while-loop+iterator.
char * -> std::string
button_info * -> std::shared_ptr<button_info>
New variables are added to replace in-place calculations.
@srifqi srifqi merged commit 2061984 into minetest:master Jul 14, 2023
13 checks passed
grorp added a commit to grorp/minetest that referenced this pull request Jul 28, 2023
+ Fix TouchScreenGUI compilation on desktop after minetest#13640
grorp added a commit to grorp/minetest that referenced this pull request Jul 28, 2023
+ Fix TouchScreenGUI compilation on desktop after minetest#13640
grorp added a commit to grorp/minetest that referenced this pull request Jul 28, 2023
+ Fix TouchScreenGUI compilation on desktop after minetest#13640
@srifqi srifqi deleted the touchscreengui_refactor branch December 1, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants