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

Increase JOY_NUM_BUTTONS to 128 and add required localizations #5960

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

derrickturk
Copy link

I want to put this out as less of a "pull request" and more of a strawman or question from a drive-by contributor - is this worth doing? Are there non-obvious downsides (performance? backward compatibility with old pilot profiles [maybe the movement of the hat switch identifiers has bad implications]? etc.).

Bumping the joystick button limit to match what I understand to be the maximum allowed by DirectInput (plus the now excellent multiple-controller support) would be a big help for those of us with "fancy" HOTAS setups. Many mid- and high-end controllers support acting as multiple "virtual controllers", but for at least some this doesn't work with FS2Open due to each "virtual controller" having the same identifiers.

@derrickturk derrickturk requested a review from z64555 as a code owner January 28, 2024 03:31
@Baezon
Copy link
Member

Baezon commented Jan 28, 2024

Have you tested whether this is sufficient to actually use buttons past 32? I assume you have such a joystick?

@derrickturk
Copy link
Author

Yes, it's worked fine with my setup (joystick + a separate throttle with ~100-odd buttons) on "my machine" (Windows 10, build from above patch on MSVC 2022, MediaVPs, Knossos.NET) in limited playtesting. I don't have the capability at the moment to test much beyond that.

@JohnAFernandez
Copy link
Contributor

So it sounds like it fixes issues for joysticks with near the max number of buttons, so I think we just need someone to test the converse: that joysticks with fewer buttons (and mice and keyboards) still work with these changes.

@JohnAFernandez JohnAFernandez added enhancement A new feature or upgrade of an existing feature to add additional functionality. controls A feature or issue related to input devices or actions controlled/triggered by them labels Jan 28, 2024
Copy link
Member

@z64555 z64555 left a comment

Choose a reason for hiding this comment

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

I had wanted to use the button name translation procedural instead of manually defining a name for each one as done here, but I suppose this is still acceptable for now.

I checked SDL's wiki to see if they had a recommended button limit but I didn't come across such a thing. It holds its numbers as a uint16_t and there's the possibility that somewhere in FSO they use the button array as a char or uint8_t. In either case the value 128 is well within the limits of the data types so I don't forsee any conflicts.

For completeness sake, we should pause merging until somebody with a more baseline joystick or controller is able to confirm that the changes don't break anything, as @JohnAFernandez suggested.

@derrickturk
Copy link
Author

Awesome, I may be able to do some testing with a Saitek Cyborg Evo (~10 buttons, IIRC) when I get back from some work travel.

@derrickturk
Copy link
Author

@z64555 what did you have in mind for a procedural translation? I like the idea but didn't feel comfortable either going from static arrays to a lookup function or introducing some kind of metaprogramming (templates or macros) to generate the static arrays. That second option is the way I would lean but I am a total rando to this codebase!

@JohnAFernandez JohnAFernandez added the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Feb 3, 2024
@JohnAFernandez JohnAFernandez marked this pull request as draft February 3, 2024 21:05
@JohnAFernandez
Copy link
Contributor

Hey, I'm just converting this to draft until we figure out if we want to make the string generation dynamic or not, so that no one merges it by accident. I'm good with it being static.

@derrickturk
Copy link
Author

I summoned something from the dreaming void. It appears to be valid C++20: https://gist.github.com/derrickturk/51c68fb15cbf82211dc4c812832487b3

I think there may be an even shakier, standards-wise, way to "count" using the preprocessor, but it's beyond me. At most I'd probably use an "X-macro" to simplify the generation of arrays entries, like so: https://gist.github.com/derrickturk/2e26b18de7f075b0b069bae63ec24df8. At least that keeps the "counting" simple and in one place, although it introduces the requirement to make sure that the count constants get updated concurrently with the definition list. I think keeping it as is would be just fine too and possibly the least confusing option for most programmers.

@derrickturk
Copy link
Author

I guess I should elaborate that the above techniques are both "static" in the sense of operating at or before compile-time and generating honest-to-god constant arrays. There would be a whole other set of approaches for generating these at runtime - probably simpler to program but at the expense of a tiny bit of performance and complexity (when do we initialize them, etc etc).

@JohnAFernandez
Copy link
Contributor

JohnAFernandez commented Feb 7, 2024

I don't think we'd be able to upgrade to c++ 20 just for this. We do have some c++ 17 features added via small libraries, but we're targeted to c++11 for now. I don't think anyone would object to adding a library to manually add a c++20 feature. The choice is up to you and @z64555 on whether that's worthwhile or not.

@derrickturk
Copy link
Author

IMO it's not worth it for the metaprogramming here - I've blown up several "C++20" compliant compilers with similar code (the core idea comes from a named-tuple implementation) so it's a bit "bleeding edge".

My personal comfort zone would be using an X-macro to keep the arrays global, static, and constant, but I don't see that pattern elsewhere in the codebase and I'd be reluctant to throw it in here as a one-off unless it's well-known to other developers.

Probably the best answers, again IMO, are to keep things as is, or perhaps introduce a singleton like io::mouse::CursorManager which generates the tables dynamically once and gets initialized "early" with a static init() function.

@Goober5000 Goober5000 removed the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Feb 26, 2024
@Goober5000 Goober5000 added the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Aug 27, 2024
@Goober5000 Goober5000 removed the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Oct 28, 2024
@Goober5000
Copy link
Contributor

PR #6371 adds the procedural translation, so the localization part of this PR is obsolete. That leaves just the number bump.

@wookieejedi
Copy link
Member

wookieejedi commented Dec 14, 2024

We had someone recently ask about increasing the number of buttons, so just wanted to check in on this PR. Looks like @z64555 has approved it. What's the remaining blockers to move it out of draft and into review/merge readiness? :)

@derrickturk
Copy link
Author

IIRC @z64555 built a proper implementation of procedurally-generated button names in a previous commit. I read some of the discussion around that and I believe the blocker on increasing JOY_NUM_BUTTONS was an interaction with the control presets code. I have not had a chance to investigate that further yet, unfortunately.

@wookieejedi
Copy link
Member

Chatting with z in Discord, at this point all that is needed for the control limit bump is "essentially its a one-line change thats blocked by needing to test if it would break anything"

wookieejedi added a commit to wookieejedi/fs2open.github.com that referenced this pull request Dec 20, 2024
Follow-up to scp-fs2open#6371 (and replaces scp-fs2open#5960 since the goal of that PR was already taken care of with scp-fs2open#6371 adding the procedural translation).

Overall tagging this as 25.0 since multiple folks have asking about bumping the limit
@wookieejedi
Copy link
Member

@derrickturk I've gone ahead and made just the bump PR separately in #6479; out of curiosity, would you be able to test that PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
controls A feature or issue related to input devices or actions controlled/triggered by them enhancement A new feature or upgrade of an existing feature to add additional functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants