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

InputSource, replaces #558 #749

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

InputSource, replaces #558 #749

wants to merge 24 commits into from

Conversation

UE4SS
Copy link
Collaborator

@UE4SS UE4SS commented Jan 8, 2025

This PR is rebased onto main.
The old one is a disaster of a PR, this one contains only the InputSource commits and is rebased onto main.
Please see #558 for details.

This PR needs testing.
Keybinds seem to work fine for me on Linux using Proton, tested with Palworld.
Someone needs to test on Windows natively.

@UE4SS-RE UE4SS-RE deleted a comment from github-actions bot Jan 8, 2025
Copy link
Contributor

github-actions bot commented Jan 8, 2025

MSVC-Game__Debug__Win64 Download Logs
Build Details
Name Information
PR Commit e6ccaad
Merge Commit bf0926d
Size 46.74 MB
Last Updated Jan 11, 25, 12:48:37 AM UTC
Expires At Jan 25, 25, 12:48:32 AM UTC

MSVC-Game__Shipping__Win64 Download Logs
Build Details
Name Information
PR Commit e6ccaad
Merge Commit bf0926d
Size 28.39 MB
Last Updated Jan 11, 25, 12:53:27 AM UTC
Expires At Jan 25, 25, 12:53:25 AM UTC

@narknon
Copy link
Collaborator

narknon commented Jan 9, 2025

How crazy would it be to make the register_keydown_event and other functions/lambdas just compile into nothing rather than wrapping every use of them in ifdefs?

@UE4SS
Copy link
Collaborator Author

UE4SS commented Jan 9, 2025

How crazy would it be to make the register_keydown_event and other functions/lambdas just compile into nothing rather than wrapping every use of them in ifdefs?

I agree.
I think it's better to wrap the contents of the functions with #ifdef instead so that it's empty when not supported.

@UE4SS
Copy link
Collaborator Author

UE4SS commented Jan 9, 2025

How crazy would it be to make the register_keydown_event and other functions/lambdas just compile into nothing rather than wrapping every use of them in ifdefs?

I agree. I think it's better to wrap the contents of the functions with #ifdef instead so that it's empty when not supported.

Although this could be tricky with functions that use a type that is also #ifdef'd out, like get_input_handler.
You'd have to dummy that type to make it compile, and then it also returns a reference which means there needs to be an #else that returns a static version of the dummy type.
It makes it more complicated to implement but removes all burden from users of the API which I think is better.

Note that get_input_handler doesn't seem to be used anywhere, and doesn't exist before this PR so it should probably be removed.

@narknon
Copy link
Collaborator

narknon commented Jan 9, 2025

Can you just ifdef the members?

@UE4SS
Copy link
Collaborator Author

UE4SS commented Jan 9, 2025

Can you just ifdef the members?

I don't see how that would help here.
If the members are ifdef'd out, anything that uses those members won't compile.

@narknon
Copy link
Collaborator

narknon commented Jan 9, 2025

Won't the only things using the members be the ifdef'd functions? Idk, I haven't looked closely at it.

@UE4SS
Copy link
Collaborator Author

UE4SS commented Jan 9, 2025

Won't the only things using the members be the ifdef'd functions? Idk, I haven't looked closely at it.

Members and functions are currently fully ifdef'd out.
As a result, users of those members and functions also need an ifdef around them or they won't compile.
This would be inconvenient for mods that register keybinds for example.
They shouldn't have to ifdef their own calls to the registration function.
If we remove get_input_handler, I think the registration functions are the only place where this matters as everything else is internal stuff.

@UE4SS
Copy link
Collaborator Author

UE4SS commented Jan 11, 2025

The discussion above has been resolved, the current status is now:

  1. Waiting for someone to test and confirm that keybinds work as expected on native Windows.
    There are build artifacts above, you don't have to build it yourself.
  2. Waiting for @Yangff to possibly have a quick look.

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