Introducing game controller rumble in SDL builds #729

LordEidi · 2021-11-11T22:31:55Z

This is a first shot at having the game controllers which can rumble react to blit::vibration in the SDL port.

LordEidi · 2021-11-11T22:43:47Z

There are a few points which probably will need some discussion:

Currently there is no update in-game. If a controller is added or removed during the lifetime of the application, the corresponding SDL events won't be handled. But I will add this once we agreed on the rest.
There is a new struct which holds the pointer to the game controller, as well as a boolean which stores the info re rumble ability of the controller. There is no need to regularly call rumble if the controller is not able to do so.
I had to split the initialisation code in 32blit-sdl/Main.cpp around line 241 since I put the controller storage into 32blit-sdl/Input.hpp. While it would not necessarily be needed to first initialise the Input class it probably would make sense to move the game controller registration (and de-reg) code into the Input class so that we could call the same code from within the SDL Event handler.
There is still a bit of fprint sprayed through the code. That can be removed later on and was mostly used for debugging purposes.
The duration of the rumble is always 20ms. Mostly because the SDL API and the 32blit API are not compatible. The one asks for a duration and the other just has a value for the rumble frequency. Telling the SDL API to rumble for 20ms with the frequency written into the blit::vibration solves the problem somewhat, that we call the rumble function for as long as the blit::vibration is set without the need to have some complex logic to sync the SDL duration with the 32blit loops.

Daft-Freak · 2021-11-12T11:59:44Z

Yeah, probably makes sense to put all the controller setup in the Input class. In the SDL code all the duration does is set the rumble to zero at a later point using the exact same API exposed to users, so you can probably just set the duration to something longer than the update period and pretend it doesn't exist.

Also, SDL just got a HasRumble query... libsdl-org/SDL#4943 (Not that we can use it yet)

... and no PR would be complete without me pointing out that the indentation is looking a bit wonky 😄

Daft-Freak · 2021-11-12T12:02:35Z

32blit-sdl/Main.cpp

+
+            SDL_GameController* gc = SDL_GameControllerOpen(n);
+
+            if (gc != nullptr && SDL_GameControllerGetAttached(gc) == SDL_TRUE) {


I'm not sure open can be successful if the controller isn't connected?

Funnily, this is taken from some SDL Lib example. But I get your point, the second part of the if could probably be omited.

Daft-Freak · 2021-11-12T12:03:22Z

32blit-sdl/Input.hpp

 class System;

+struct game_controller


Should be CamelCase I think. (Maybe even private to Input)

Thought about making it private as well. But since not all functions are within the Input class, it currently needs to be public.

And re CamelCase, nopes, see file.hpp (32blit main, not SDL part)

The struct in that file is CamelCase though? I know there's packed_image, but I think that was missed back when everything was changed to CamelCase...

Daft-Freak · 2021-11-12T12:06:12Z

32blit-sdl/Main.cpp

-	blit_input = new Input(blit_system);
+    if(!blit_input->game_controllers.empty()) {
+
+        SDL_GameControllerEventState(SDL_ENABLE);


Hmm, enabled should be the default.

Taken from an example as well. Not sure it always is. Documentation is not 100% clear on that. Would leave this as it is.

I'm pretty sure that there are only a few SDL events that are disabled by default, and they're not controller events... also it worked without that before.

Daft-Freak · 2021-11-12T12:08:22Z

32blit-sdl/Main.cpp

+        SDL_GameControllerClose(gc.gc_id);
+    }
+    blit_input->game_controllers.clear();
+


I'd say that should be in Input's destructor... but it's currently not getting deleted. Something else to fix I guess...

Indeed. Leaving there until fixed, or moving to the Input class and calling it from here?

I'd just stick a delete with the other three below, fixing the (tiny) leak too. (Not that it matters much as we're exiting anyway...)

Daft-Freak · 2021-11-12T12:12:02Z

32blit-sdl/System.cpp

@@ -269,6 +272,16 @@ void System::loop()
 	blit::joystick.y = shadow_joystick[1];
 	SDL_UnlockMutex(m_input);
 	blit::tick(::now());
+
+    if(blit::vibration > 0) {


Doesn't this mean you can't turn vibration off? Though the duration would make it auto-disable anyway... This block could be an update_rumble method on Input or something similar.

Nopes, no need to. The code only rumbles for 20ms max anyway. So it only rumbles until the next cycle. And then either is restarted for another 20ms or silenced automatically.

LordEidi · 2021-11-12T12:47:17Z

Yeah, probably makes sense to put all the controller setup in the Input class. In the SDL code all the duration does is set the rumble to zero at a later point using the exact same API exposed to users, so you can probably just set the duration to something longer than the update period and pretend it doesn't exist.

re Input class. OK, I am moving the code into the Input class.

re duration. Not sure I get what you mean. But that is what is happening now. Rumble is set for 20ms which is one cycle. And is then restarted for another 20ms if still asked for by the game. No need to stop, you won't have any chance to cancel those 20ms anyway.

Also, SDL just got a HasRumble query... libsdl-org/SDL#4943 (Not that we can use it yet)

Thanks for the hint. The documentation regarding controllers is a bit difficult btw.

... and no PR would be complete without me pointing out that the indentation is looking a bit wonky smile

Must be the problem with the diff between me writing and me moving the changes into another repository... Looking into it, but I hope you can life with it anyway.

Daft-Freak · 2021-11-12T12:59:43Z

For the duration I was mostly pointing out that setting the rumble again before the duration is finished is okay, because SDL itself does that to stop after the duration is finished. (It might be a good idea to set a duration slightly higher than the update interval, since there's no guarantee that the 20ms timer is actually 20ms.)

For the indentation, it's apparently supposed to be 2 spaces (https://github.com/32blit/32blit-sdk/blob/master/.editorconfig#L11-L12). But the SDL code is mostly tabs... Though the new code here is neither of those.

LordEidi · 2021-11-12T13:17:43Z

What should I take as duration? 50ms? Not noticeable by the user, but long enough for the cycle duration?

Daft-Freak · 2021-11-12T13:27:57Z

Something like that yeah, enough to cover any timing variations but doesn't need to be too big.

LordEidi · 2021-11-12T14:32:44Z

Hopefully I addressed all points:

changed 20ms to 50ms.
moved all controller handling into the Input class.
added a destructor to Input and calling delete from Main.
added correct event handling for removed and added controllers.
configured 1 tab to 2 spaces.
renamed struct to CamelCase.
removed unwanted / not needed code.

Daft-Freak · 2021-11-12T15:39:34Z

Well, it works... when you use a controller that actually has rumble 🤦 (turns out most of the controllers I have lying around don't...)

Don't see anything non-trivial to complain about so I'll just ignore any little formatting things and leave the rest to someone who can press the merge button 😄

LordEidi · 2021-11-12T15:56:38Z

Thanks @Daft-Freak for your support!

cgutman · 2021-11-12T19:08:09Z

32blit-sdl/Input.cpp

+
+void Input::_add_controller(SDL_GameController* gc) {
+  // welcome rumble to test if it can rumble
+  auto can_rumble = SDL_GameControllerRumble(gc, 0xFFFF, 0xFFFF, 200);


If you don't actually want the welcome rumble, you can probe for rumble functionality using short low-intensity rumble like SDL_GameControllerRumble(gc, 1, 1, 1) for versions of SDL that lack SDL_GameControllerHasRumble(). It's so short that the rumble motors won't even have time to spin up.

Gadgetoid · 2022-01-06T15:16:47Z

What's the status on this? I'd love to merge it for the next release.

LordEidi · 2022-01-06T15:38:05Z

What's the status on this? I'd love to merge it for the next release.

You should be good to go. Just added the changes to the 32blit dev version yesterday.

Although: There is a "welcome rumble" which shortly rumbles the controller when being initialised. You should reduce the duration to 1 if you do not want that to be noticed by the users. I find it quite useful since you know things work because of the welcome greeting. But YMMV.

Gadgetoid · 2022-01-06T16:15:14Z

I find it quite useful since you know things work because of the welcome greeting.

It does seem like a helpful debugging thing. I'd probably add a command-line switch to enable/disable it in future.

Gadgetoid · 2022-01-06T16:24:55Z

And... merged! Thank you all involved.

Introducing game controller rumble

1bd7b73

LordEidi changed the title ~~Introducing game controller rumble~~ Introducing game controller rumble in SDL builds Nov 11, 2021

Daft-Freak reviewed Nov 12, 2021

View reviewed changes

LordEidi added 2 commits November 12, 2021 15:25

Rumble, take 2

2ec6bc5

Rumble, take 2

5704f77

cgutman reviewed Nov 12, 2021

View reviewed changes

Gadgetoid merged commit 1204303 into 32blit:master Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing game controller rumble in SDL builds #729

Introducing game controller rumble in SDL builds #729

LordEidi commented Nov 11, 2021

LordEidi commented Nov 11, 2021

Daft-Freak commented Nov 12, 2021

Daft-Freak Nov 12, 2021

LordEidi Nov 12, 2021

Daft-Freak Nov 12, 2021

LordEidi Nov 12, 2021

LordEidi Nov 12, 2021 •

edited

Loading

Daft-Freak Nov 12, 2021

Daft-Freak Nov 12, 2021

LordEidi Nov 12, 2021

Daft-Freak Nov 12, 2021

Daft-Freak Nov 12, 2021

LordEidi Nov 12, 2021

Daft-Freak Nov 12, 2021

Daft-Freak Nov 12, 2021

LordEidi Nov 12, 2021

LordEidi commented Nov 12, 2021

Daft-Freak commented Nov 12, 2021

LordEidi commented Nov 12, 2021

Daft-Freak commented Nov 12, 2021

LordEidi commented Nov 12, 2021

Daft-Freak commented Nov 12, 2021

LordEidi commented Nov 12, 2021

cgutman Nov 12, 2021

Gadgetoid commented Jan 6, 2022

LordEidi commented Jan 6, 2022

Gadgetoid commented Jan 6, 2022

Gadgetoid commented Jan 6, 2022


		SDL_GameController* gc = SDL_GameControllerOpen(n);

		if (gc != nullptr && SDL_GameControllerGetAttached(gc) == SDL_TRUE) {

Introducing game controller rumble in SDL builds #729

Introducing game controller rumble in SDL builds #729

Conversation

LordEidi commented Nov 11, 2021

LordEidi commented Nov 11, 2021

Daft-Freak commented Nov 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LordEidi Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LordEidi commented Nov 12, 2021

Daft-Freak commented Nov 12, 2021

LordEidi commented Nov 12, 2021

Daft-Freak commented Nov 12, 2021

LordEidi commented Nov 12, 2021

Daft-Freak commented Nov 12, 2021

LordEidi commented Nov 12, 2021

Choose a reason for hiding this comment

Gadgetoid commented Jan 6, 2022

LordEidi commented Jan 6, 2022

Gadgetoid commented Jan 6, 2022

Gadgetoid commented Jan 6, 2022

LordEidi Nov 12, 2021 •

edited

Loading