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

Linux game mode esc exit hotfix #16074

Merged
merged 2 commits into from Jun 12, 2023

Conversation

nicholas-rh
Copy link
Contributor

This is a temporary hotfix (may not be the ideal solution) for the current issue of not being able to exit the game mode in Linux after entering. Opening a PR for it to gather feedback.

What does this PR do?

Allow for exiting from game mode using ESC as intended.

Please add links to any issues, RFCs or other items that are relevant to this PR.
#15958

How was this PR tested?

Tested on Fedora 38, by checking to see if exiting game mode was possible by pressing ESC.

@nicholas-rh nicholas-rh requested review from a team as code owners June 1, 2023 17:21
@nicholas-rh nicholas-rh marked this pull request as draft June 1, 2023 17:23
This is a hotfix (may not be the ideal solution) for the current issue of not
being able to exit the game mode in Linux after entering.

Signed-off-by: Nicholas Frizzell <nfrizzel@redhat.com>
@nicholas-rh nicholas-rh marked this pull request as ready for review June 2, 2023 12:08
Copy link
Contributor

@michalpelka michalpelka left a comment

Choose a reason for hiding this comment

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

I've cherry-picked changes and tested them. It works as expected.

Copy link
Contributor

@nick-l-o3de nick-l-o3de left a comment

Choose a reason for hiding this comment

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

It looks okay to me. It also sounds like this fix is considered a temp fix? It may be helpful to either open an issue about what might be the "real" full fix and link it here, so that it doesn't get lost.

My remaining concern would be how this behaves on other operating systems besides linux. If linux was the odd one out, adding this code in a non-linux section would affect the others, which were presumably working.

@AMZN-alexpete
Copy link
Contributor

Is the issue that IMGUI is swallowing the input, or that the ESC key input is not correctly captured? IMGUI at one time, was not a required gem, in which case, would projects without IMGUI gem enabled still have this issue?

@mbalfour-amzn
Copy link
Contributor

@kberg-amzn is this possibly related to #15891 ?

@nicholas-rh
Copy link
Contributor Author

Is the issue that IMGUI is swallowing the input, or that the ESC key input is not correctly captured? IMGUI at one time, was not a required gem, in which case, would projects without IMGUI gem enabled still have this issue?

When testing commit 9b07c7c (parent commit of this PR commit) and manually removing ImGui from project.json + rebuilding, the problem of not being to exit game mode still seems to happen, which possibly lends credence to the idea that ESC isn't being captured where it should be outside of any influence from ImGui. It's also possible there is other ImGui code still hanging around though (seems to be some hooks in Atom for example). It seems like there are several places which listen for an ESC key press and try to exit game mode, but when testing with lldb and setting breakpoints on them I was never able to get them to trigger.


if (inputChannel.GetInputChannelId() == AzFramework::InputDeviceKeyboard::Key::Escape)
{
CSystem *cSys = (CSystem*)GetISystem();
Copy link
Contributor

Choose a reason for hiding this comment

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

standards: static_cast should be preferred here instead of a C-style cast according to the coding standards: https://github.com/o3de/sig-core/blob/main/governance/Coding-Standards-and-Style-Guide.md#casting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 368d220

Signed-off-by: Nicholas Frizzell <nfrizzel@redhat.com>
@AMZN-alexpete AMZN-alexpete requested a review from kberg0 June 7, 2023 00:47
@nicholas-rh
Copy link
Contributor Author

nicholas-rh commented Jun 7, 2023

It looks okay to me. It also sounds like this fix is considered a temp fix? It may be helpful to either open an issue about what might be the "real" full fix and link it here, so that it doesn't get lost.

My remaining concern would be how this behaves on other operating systems besides linux. If linux was the odd one out, adding this code in a non-linux section would affect the others, which were presumably working.

Sorry for the delay I haven't forgotten about this, I set up a Windows guest VM but was running into some problems so I'll double check everything for compatibility once I get an actual installation up and running.

As far as temp vs. permanent fix, for this solution I mostly copied the preexisting key press event logic which was removed from the XConsole code into the equivalent ImGui handler. From a bigger architectural standpoint it may or may not actually belong there since there are other points in the code where an ESC key press to exit game mode seem to be attempted to be handled. If it doesn't then I imagine there are some other larger changes that would need to be made to rectify this. In the meantime this seems to work as a solution on Linux.

@kberg0
Copy link
Contributor

kberg0 commented Jun 9, 2023

@kberg-amzn is this possibly related to #15891 ?

It's possible. I removed extra imgui and legacy cry console key binding in that change. I didn't add any new key binding that might swallow input unexpectedly, but that invisible cry console was definitely ticking and doing strange things like capturing all inputs and making it appear the player had lost all input binding. It could possibly still have been critical path for escape key handling and I missed it during testing.

@adamdbrw adamdbrw merged commit d5a8cc2 into o3de:development Jun 12, 2023
3 checks passed
@spham-amzn spham-amzn added the sig/platform Categorizes an issue or PR as relevant to SIG Platform. label Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/platform Categorizes an issue or PR as relevant to SIG Platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants