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.

Sign up for GitHub

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

Jump to bottom

Add SDL input samples #106

Closed
wants to merge 3 commits into from
Closed

Add SDL input samples #106

wants to merge 3 commits into from

Conversation

dracc
Copy link
Contributor

@dracc dracc commented May 7, 2019 •
edited
Loading

Samples which requires XboxDev/nxdk-sdl#5.

@dracc dracc mentioned this pull request May 7, 2019
Merged
@thrimbor
Copy link
Member

thrimbor commented May 8, 2019

I recommend delaying this until the SDL work has landed (and this commit was rebased). I'd rather have a non-bisectable commit in a submodule than in nxdk.

@dracc dracc changed the title sdl: Add joystick to SDL Makefile sdl: Move SDL Makefile into the SDL repository May 8, 2019
@dracc dracc force-pushed the sdl_joystick branch 3 times, most recently from 24d659c to 767b119 Compare May 9, 2019 14:14
@dracc dracc mentioned this pull request May 11, 2019
Merged
@dracc dracc force-pushed the sdl_joystick branch 2 times, most recently from bb1a366 to 8d25562 Compare May 12, 2019 07:42
@dracc dracc changed the title sdl: Move SDL Makefile into the SDL repository Move SDL Makefile into the SDL repo, add input samples May 12, 2019
@dracc dracc changed the title Move SDL Makefile into the SDL repo, add input samples Move SDL Makefile into SDL repo, add input samples May 12, 2019
@dracc dracc force-pushed the sdl_joystick branch 2 times, most recently from 61a474f to a0f35be Compare July 27, 2019 20:55
thrimbor
thrimbor reviewed Aug 22, 2019
View reviewed changes
samples/sdl_gamecontroller/Makefile Outdated Show resolved Hide resolved
samples/sdl_gamecontroller/main.c Outdated Show resolved Hide resolved
samples/sdl_gamecontroller/main.c Outdated Show resolved Hide resolved
JayFoxRox
JayFoxRox reviewed Aug 28, 2019
View reviewed changes
samples/sdl_joystick_event/main.c
SDL_QuitSubSystem(SDL_INIT_JOYSTICK);
XSleep(2000);
pb_kill();
break;
Copy link
Member

Choose a reason for hiding this comment

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

If this breaks, it will run the same deinit code again outside of the main-loop.
If you don't break, you probably still want to close the joy joystick.

JayFoxRox
JayFoxRox reviewed Aug 28, 2019
View reviewed changes
samples/sdl_joystick_event/main.c
int ret = pb_init();
if (ret != 0) {
XSleep(2000);
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

Negative numbers returned from main are very uncommon.

JayFoxRox
JayFoxRox reviewed Aug 28, 2019
View reviewed changes
samples/sdl_gamecontroller_event/main.c
SDL_QuitSubSystem(SDL_INIT_GAMECONTROLLER);
XSleep(2000);
pb_kill();
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

See same comment in joystick sample about de-init after main-loop and closing of gameController.

JayFoxRox
JayFoxRox reviewed Aug 28, 2019
View reviewed changes
samples/sdl_gamecontroller/main.c
while (true) {
// Fetch current GameController state
SDL_GameControllerUpdate();
debugClearScreen();
Copy link
Member

Choose a reason for hiding this comment

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

The existing review comments, about checking wether gameController is still connected, seem to have been ignored / missed.

Consider https://wiki.libsdl.org/SDL_GameControllerGetAttached?highlight=%28%5CbCategoryGameController%5Cb%29%7C%28CategoryEnum%29

@JayFoxRox
Copy link
Member

Shall we:

  • Merge this as-is, despite being subpar quality (for being a reference-sample, for an already well-documented SDL API), with fixes in the future? The risk is non-ideal application quality based on these samples and additional code to maintain with future changes.

..or..

  • Should we close this; potentially spending more time on review on resubmission?

@dracc
Copy link
Contributor Author

dracc commented Sep 30, 2019

I'll pull the trigger on closing unless someone objects.

@dracc
Copy link
Contributor Author

dracc commented Sep 30, 2019 •
edited
Loading

Closed as my design was bad and my implementation of this bad design was even worse. 👍

@dracc dracc closed this Sep 30, 2019
@dracc dracc deleted the sdl_joystick branch September 30, 2019 20:35
@JayFoxRox JayFoxRox mentioned this pull request Sep 30, 2019
Open
@dracc dracc restored the sdl_joystick branch September 30, 2019 20:57
@dracc dracc deleted the sdl_joystick branch August 10, 2020 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RadWolfie RadWolfie RadWolfie left review comments

@thrimbor thrimbor thrimbor left review comments

@JayFoxRox JayFoxRox JayFoxRox left review comments

@mborgerson mborgerson Awaiting requested review from mborgerson

@LukeUsher LukeUsher Awaiting requested review from LukeUsher

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dracc @thrimbor @JayFoxRox @RadWolfie