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

fix joystick events in WaitEventTimeout, fix HapticRumbleSupported return value #238

Merged
merged 2 commits into from
Dec 31, 2024

Conversation

Kethen
Copy link
Contributor

@Kethen Kethen commented Dec 31, 2024

I think SDL_PROP_JOYSTICK_CAP_MONO_LED_BOOLEAN should be mapped to JoystickHasLED true, but I'm unsure

update: not changing JoystickHasLED anymore

@sezero sezero requested review from slouken and icculus December 31, 2024 11:33
src/sdl3_syms.h Outdated
@@ -429,7 +432,7 @@ SDL3_SYM_RENAMED_RETCODE(bool,HapticPause,PauseHaptic,(SDL_Haptic *a),(a),return
SDL3_SYM_RENAMED_RETCODE(bool,HapticRumbleInit,InitHapticRumble,(SDL_Haptic *a),(a),return)
SDL3_SYM_RENAMED_RETCODE(bool,HapticRumblePlay,PlayHapticRumble,(SDL_Haptic *a, float b, Uint32 c),(a,b,c),return)
SDL3_SYM_RENAMED_RETCODE(bool,HapticRumbleStop,StopHapticRumble,(SDL_Haptic *a),(a),return)
SDL3_SYM_PASSTHROUGH_RETCODE(bool,HapticRumbleSupported,(SDL_Haptic *a),(a),return)
SDL3_SYM_PASSTHROUGH_INT_BOOL(bool,HapticRumbleSupported,(SDL_Haptic *a),(a),return)
Copy link
Contributor

Choose a reason for hiding this comment

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

How did this not error before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SDL3_SYM_PASSTHROUGH_RETCODE matches type but returns 0 and -1, which flips the outcome

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be changed to a wrapper function rather than adding a new passthrough macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented SDL_HapticRumbleSupported as a wrapper function instead

Copy link
Contributor

Choose a reason for hiding this comment

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

SDL3_SYM_PASSTHROUGH_RETCODE matches type but returns 0 and -1, which flips the outcome

I wonder whether there are any other places where we screwed up like that. (I hope not..)

return SDL3_GetBooleanProperty(SDL3_GetJoystickProperties(joystick), SDL_PROP_JOYSTICK_CAP_RGB_LED_BOOLEAN, false) ? SDL2_TRUE : SDL2_FALSE;
bool has_led = SDL3_GetBooleanProperty(SDL3_GetJoystickProperties(joystick), SDL_PROP_JOYSTICK_CAP_RGB_LED_BOOLEAN, false) ||
SDL3_GetBooleanProperty(SDL3_GetJoystickProperties(joystick), SDL_PROP_JOYSTICK_CAP_MONO_LED_BOOLEAN, false);
return has_led ? SDL2_TRUE : SDL2_FALSE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

SDL_JoystickHasLED() in SDL2 only returned true for RGB LED support. This change should be dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

retracted the commit changing JoystickHasLED

@Kethen Kethen changed the title fix joystick events in WaitEventTimeout, fix HapticRumbleSupported return value, change JoystickHasLED fix joystick events in WaitEventTimeout, fix HapticRumbleSupported return value Dec 31, 2024
@sezero sezero requested a review from slouken December 31, 2024 20:55
@sezero
Copy link
Contributor

sezero commented Dec 31, 2024

@slouken: is this good now?

@slouken slouken merged commit 89b2d62 into libsdl-org:main Dec 31, 2024
10 checks passed
@slouken
Copy link
Collaborator

slouken commented Dec 31, 2024

Merged, thanks!

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