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

Make rumble functions configurable #2742

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

Conversation

n0toose
Copy link

@n0toose n0toose commented Jan 24, 2024

Affects GameControllerManager::rumble() and JoystickManager::rumble().

The functions were overloaded so as to be made configurable, but there
is a default rumble (that now lasts 100ms instead of 300ms).
Just a small refactoring job.


I have not figured out how to expose the rumble function to Player.cpp in a clean way yet, but this is a start.

Related to #2741.

@n0toose
Copy link
Author

n0toose commented Jan 24, 2024

Wait, I completely forgot about joysticks (and left in a mistake during a rebasing mistake) that skips most of the conditions.

@n0toose n0toose marked this pull request as draft January 24, 2024 15:44
@n0toose n0toose marked this pull request as ready for review January 24, 2024 15:56
@n0toose
Copy link
Author

n0toose commented Jan 24, 2024

Should hopefully be good to go? (Note: I don't own a joystick, so the changes affecting the joystick component are untested - even if they are presumed to be fine.)

@n0toose n0toose changed the title Overload GameControllerManager::rumble to make it configurable Make rumble functions configurable Jan 24, 2024
@mrkubax10 mrkubax10 added type:feature category:code status:needs-review Work needs to be reviewed by other people labels Jan 26, 2024
src/control/game_controller_manager.cpp Outdated Show resolved Hide resolved
src/control/game_controller_manager.hpp Outdated Show resolved Hide resolved
JoystickManager::rumble(SDL_Joystick* controller) const
JoystickManager::rumble(SDL_Joystick* joystick) const
{
return this->rumble(joystick, 0xFFFF, 0xFFFF, 100);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Comment on lines 21 to 25
#include "control/controller.hpp"

#include <stdint.h>
#include <vector>
#include <unordered_map>

#include "control/controller.hpp"
#include <SDL.h>
Copy link
Member

Choose a reason for hiding this comment

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

Same as previously too.

@mrkubax10
Copy link
Member

Besides my previous comments maybe it would make sense to add rumble configuration to options menu? Because currently this change seems little bit pointless.

@n0toose
Copy link
Author

n0toose commented Jan 27, 2024

Because currently this change seems little bit pointless.

I haven't completed my work in trying to get GameControllerManager to communicate with Player yet - but I thought that I should send this very small refactor (which was the only working thing I got after completing my coding session) so that it's easier to pick it up again later or let someone else pick it up if I abandon it.

Besides my previous comments maybe it would make sense to add rumble configuration to options menu?

I do not know if I should do that yet, as that would require me to answer the following: Should I use the GameControllerManager class for storing these settings, instead of Player? I do not have a perfect understanding of how these components work together, and would appreciate some assistance.

Later on, I'd like to use information such as the direction that a player was hit from or whether the player got a fire flower more easily later.

@MatusGuy
Copy link
Member

I think you should trigger rumble when there's any type of screenshake, which does not require player

@n0toose
Copy link
Author

n0toose commented Jan 27, 2024

I think you should trigger rumble when there's any type of screenshake, which does not require player

Hm, would be a good start and one could immediately combine it with #2741.

@n0toose n0toose marked this pull request as draft January 27, 2024 23:33
Affects GameControllerManager::rumble() and JoystickManager::rumble().

The functions were overloaded so as to be made configurable, but there
is a default rumble (that now lasts 100ms instead of 300ms).
Just a small refactoring job.
@n0toose
Copy link
Author

n0toose commented Jan 28, 2024

I think you should trigger rumble when there's any type of screenshake, which does not require player

I'll look at whether I can get the Camera class to talk to GameControllerManager.

@MatusGuy
Copy link
Member

MatusGuy commented Feb 1, 2024

it's not a singleton?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:code status:needs-review Work needs to be reviewed by other people type:feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants