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

Several fixes and improvements... #134

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

KoopaOnGit
Copy link

@KoopaOnGit KoopaOnGit commented Aug 27, 2024

  • Added ability to arbritrarily block/unblock keyboard input at any time, fixes a 'white screen' issue when pressing ESC right at the time where the save screen is shown - on Linux this even segfaults for some reason, haven't investigated root cause but this definitely seems to reliably stop it from happening on a higher level.

  • Usage of SAFE_DELETE when SAFE_DELETE_ARRAY should be used for the sake of correctness

  • Zero-init float arrays in FireTexture constructors (I forgot why I did this, there was a reason I believe)

  • Fix OOB read during Font::init when initializing a Font that may not exist or has corrupted data (both VS-ASan and Dr.Memory complained about it)

@@ -1163,6 +1163,8 @@ int Minecraft::getFpsIntlCounter()

void Minecraft::leaveGame(bool bCopyMap)
{
Keyboard::setBlocked(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

When is the keyboard unblocked?

Copy link
Author

Choose a reason for hiding this comment

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

Keyboard::reset() unblocks it - So, no worries...

@TheBrokenRail
Copy link
Contributor

IMO, instead of just disabling the keyboard arbitrarily while leaving the world, it would be better to determine (and fix) the actual bugs instead of just hiding them.

@KoopaOnGit
Copy link
Author

You can do so by getting rid of the keyboard blocking code and reproducing the bug - but to be honest, I couldn't debug the root issues as I remember because:

  1. On Windows it only gives out a weird white screen and I have no prior experience towards debugging something like this as I lack knowledge of how to trace back rendering related issues, or I did just really try and did not want to bother.

  2. The only linux platform where I have ReMCPE working is the raspberry pi, and on there it actually has a segfault when you try to do what I stated above, yet debugging and applying patches on the Pi is really tedious so I didn't really bother either...

I think I'll try to assess the root causes and uncomment it myself and provide a patch here too now, as I guess I'll really try this time.

@KoopaOnGit
Copy link
Author

Correction: Or I did just really not try* - but I'll try now...

@KoopaOnGit
Copy link
Author

I identified the root cause
image

Commenting this out and the issue no longer persists. But here's the question: What is this even used for?

* Fix root issue for white-screen issue when pressing ESC during world saves
@iProgramMC
Copy link
Member

iProgramMC commented Aug 27, 2024

I identified the root cause [image]

Commenting this out and the issue no longer persists. But here's the question: What is this even used for?

This is used to exit out of menus, such as the pause menu, or inventory menu. I believe you have to override the Screen::keyPressed(int) method in the SavingWorldScreen to prevent the white screen issue.

@KoopaOnGit
Copy link
Author

Or just block all keyboard input during those scenes where any input isn't supposed to be handled anyway and call it a day?

@iProgramMC
Copy link
Member

Can you put the following changes in a new PR?

  • The fix to change SAFE_DELETE(m_pTiles) to SAFE_DELETE_ARRAY(m_pTiles) in StartMenuScreen.cpp
  • The fix to change SAFE_DELETE(m_data1) to SAFE_DELETE_ARRAY(m_data1) in FireTexture.cpp (same for m_data2)

The keyboard block functionality I completely disagree with. The underlying bugg can just be fixed.

https://github.com/ReMinecraftPE/mcpe/pull/134/files#diff-b6f242667aac4d5605d75f5cfc05d95bc2b2487299360c9c7bb7a10bfc3148aaR42-R48
The font code should be rearchitected to scale with the texture size.

@iProgramMC
Copy link
Member

Or just block all keyboard input during those scenes where any input isn't supposed to be handled anyway and call it a day?

Sure but this is just a bandage fix. Simply fix the root cause and we're good to go.

@KoopaOnGit
Copy link
Author

Ok. I personally thought it's OK since it's just client dev - if this was a server or any other mission-critical software then having a 1:1 ratio of fixing the actual root causes of bugs of course would be the one and only way to go.

But I agree. I'll make a new PR in a bit!

@KoopaOnGit
Copy link
Author

WAIT. Hold on, what about te OOB fixes? Push them or not?!
They only happen when you don't provide the correct assets anyway, but OOB stuff is always good to just get rid of regardless of what state triggered it.

@iProgramMC
Copy link
Member

I said that the font texture width generation needs to be rearchitected already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants