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

Rumble cvar and fixes #184

Merged
merged 15 commits into from
Apr 26, 2022
Merged

Rumble cvar and fixes #184

merged 15 commits into from
Apr 26, 2022

Conversation

Sirius902
Copy link
Contributor

@Sirius902 Sirius902 commented Apr 18, 2022

This pull request closes #182 and closes #170 by adding a rumble cvar. While the cvar is enabled, the game will think the rumble pak is inserted and SoH will vibrate the controller normally. When disabled, the game will think there is no rumble pak and SoH will not rumble the controller.

This PR also fixes another problem by rumbling the controller indefinitely until the boolean is set to false instead of doing 1ms rumbles like the previous implementation. This fixes the issue of rumble not staying on consistently during vibration. Accordingly, this PR stops rumble when SoH exits.

The reason I drafted this PR is because I want to also address #183 and get it into the PR but currently I am not sure how to fix it.

@Sirius902 Sirius902 marked this pull request as draft April 18, 2022 06:17
@jbodner09
Copy link
Contributor

Technically, adding the CVar doesn't actually fix #170. The problem shown there with the fishing game is that it specifically checks the pakType to determine if a rumble pack is inserted, and controllers with rumble capabilities aren't recognized as having rumble capabilities because there's no code for modern controllers that specifically sets the pakType as a rumble pack.
My plan was to add an OTRGlobal to get what type of controller is currently connected, and then have padmgr call that during its init, and if it's an SDL controller, set the pack type to be a rumble pak then.

Then, you'd just need to make

u32 func_800AA148(void) {
return gPadMgr.pakType[0] == 1;
}

into

u32 func_800AA148(void) {
return (gPadMgr.pakType[0] == 1) && (CVar_GetS32("gRumbleEnabled", 0) != 0);
}

for the rumble detection to also appropriately be controlled by the CVar and have the fishing guy correctly tell you rumble is disabled when rumble is manually disabled.

@Sirius902
Copy link
Contributor Author

Sirius902 commented Apr 18, 2022

@jbodner09 This does actually fix the fishing minigame. Because I set the padStatus.status to 1 when the CVar is enabled the game changes the pakType to 1 naturally. I have tested in-game and the rumble pack message shows normally. The idea here is to let the user decide to have rumble enabled or not, with it enabled the controller with rumble and the game will recognize the rumble pak is inserted.

@V10lator
Copy link

I think you're missing to CVar_RegisterS32() the "gRumbleEnabled" CVar at soh/soh/Enhancements/bootcommands.c - else I can confirm this fixes #170

@jbodner09
Copy link
Contributor

Interesting! I added padMgr->padStatus[0].status = 1; to my local build in the same place you have it here, and it didn't change the fishing message, but if multiple people are testing the change as a whole and it works, then who am I to argue?

I still think the game should be able to detect the rumble pack on its own without being told to with the CVar though. That way if you have the CVar enabled but no rumblable controller connected, you'll still get the correct fishing message. We could open another issue for that though.

@MegaMech
Copy link
Contributor

Reading through this, I think jbodner's method of implementation is way better than a cvar. Would someone actually desire to turn off rumble if it was enabled? Do most games have an option for turning off rumble?

@Sirius902
Copy link
Contributor Author

Interesting! I added padMgr->padStatus[0].status = 1; to my local build in the same place you have it here, and it didn't change the fishing message, but if multiple people are testing the change as a whole and it works, then who am I to argue?

I still think the game should be able to detect the rumble pack on its own without being told to with the CVar though. That way if you have the CVar enabled but no rumblable controller connected, you'll still get the correct fishing message. We could open another issue for that though.

You need to also change osMotorInit to return 0 otherwise the N64 will think the rumble pak is not connected.

@Sirius902
Copy link
Contributor Author

Sirius902 commented Apr 18, 2022

Reading through this, I think jbodner's method of implementation is way better than a cvar. Would someone actually desire to turn off rumble if it was enabled? Do most games have an option for turning off rumble?

Yes, most games do have an option to disable rumble and there is an open issue requesting a feature to toggle it. I could however add different modes for rumble in this PR though. One could be Auto where the game would enable it based on the controller's capabilities, Enabled for unconditionally enabled, and Disabled for the opposite. Though, I'm not really sure why somebody would like the game to think the rumble pak is connected if their controller doesn't support it, so maybe Auto and Disabled would be fine.

@MegaMech
Copy link
Contributor

Auto and disabled sounds great to me

@briaguya-ai
Copy link
Contributor

briaguya-ai commented Apr 18, 2022

Though, I'm not really sure why somebody would like the game to think the rumble pak is connected if their controller doesn't support it

only reason i can think of is fisherman text

Oh, no! You don't have a Rumble Pak! With a Rumble Pak, you can feel the vibrations of a fish on your hook. This time, no rumble for you!

vs

Wow! You have a Rumble Pak! Today, you can feel the vibration, young man!

so if someone wanted to save a few frames but not feel rumble (or test stuff out) it could be useful, but that's an edge case at best

auto and disabled seems like the move

@Sirius902
Copy link
Contributor Author

Alright, I have implemented the suggestion. Even if the rumble checkbox is enabled, the game will only think the rumble pak is inserted if the controller also supports rumble.

I'm not sure if #183 is out of scope of this PR or not since it seems related to the core game loop so I think it's probably best to see if a maintainer can weigh in on that. If it seems out of scope, then this PR is ready to be undrafted.

@Sirius902
Copy link
Contributor Author

Given that #189 has been opened and aims to address #183, I'll mark this PR as ready for review.

@Sirius902 Sirius902 marked this pull request as ready for review April 19, 2022 19:26
soh/src/code/padmgr.c Outdated Show resolved Hide resolved
soh/src/code/padmgr.c Outdated Show resolved Hide resolved
@Kenix3
Copy link
Collaborator

Kenix3 commented Apr 21, 2022

Reading through this, I think jbodner's method of implementation is way better than a cvar. Would someone actually desire to turn off rumble if it was enabled? Do most games have an option for turning off rumble?

I can see a desire for a slider to set the force amount that the rumble motor uses.

Copy link
Collaborator

@Kenix3 Kenix3 left a comment

Choose a reason for hiding this comment

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

We should probably expose the ability to set rumble settings per controller. There can be a new section in the UI for input settings.

@Sirius902
Copy link
Contributor Author

We should probably expose the ability to set rumble settings per controller. There can be a new section in the UI for input settings.

In that case, what exactly should be in the per-controller config? Should just the rumble strength be configurable per-controller and have whether or not rumble is enabled be a global setting? (This makes the most sense to me).

There is already a rumble intensity slider in the controller menu so I can try and make that affect the per-controller setting for the current controller.

@jbodner09
Copy link
Contributor

Should just the rumble strength be configurable per-controller and have whether or not rumble is enabled be a global setting?

Makes the most sense to me too. The existing slider goes from 0 to 100%, so if anybody wants to turn off rumble entirely for just a single controller, they can just drop its slider down to 0.

@Kenix3
Copy link
Collaborator

Kenix3 commented Apr 24, 2022

We should probably expose the ability to set rumble settings per controller. There can be a new section in the UI for input settings.

In that case, what exactly should be in the per-controller config? Should just the rumble strength be configurable per-controller and have whether or not rumble is enabled be a global setting? (This makes the most sense to me).

There is already a rumble intensity slider in the controller menu so I can try and make that affect the per-controller setting for the current controller.

Ah I forgot we already had a slider. I'd just change the slider to be per controller, but when the controller is at zero strength it is treated as off/disconnected for that controller. I wouldn't be opposed to a global checkmark that disables all four sliders if rumble is off globally. Then internally rumble is treated as disconnected.

@Sirius902 Sirius902 marked this pull request as draft April 24, 2022 22:13
@Sirius902 Sirius902 marked this pull request as ready for review April 25, 2022 01:20
@Sirius902
Copy link
Contributor Author

Alright, I went ahead and added controller specific settings for rumble strength (and threw in the gyro info as well for fun). These can be configured from the Controllers menu. I had to jump through some hoops to get it to work but it seems fine, just had to add some member functions to the controllers.

@Sirius902 Sirius902 requested a review from Kenix3 April 25, 2022 01:22
@jbodner09
Copy link
Contributor

FYI, this is going to conflict hard with #220 if it ends up merging before this does, and if it doesn't, then you'll need to follow up to remove the same kinds of redundant parameters that it does.

Copy link
Collaborator

@Kenix3 Kenix3 left a comment

Choose a reason for hiding this comment

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

Merge pending conflict resolution.

@Sirius902
Copy link
Contributor Author

@Kenix3 Resolved merge conflict. Also removed some unnecessary const_casts added in recent commits as well.

@NEstelami NEstelami merged commit eea5135 into HarbourMasters:develop Apr 26, 2022
@Sirius902 Sirius902 deleted the rumble branch April 27, 2022 00:02
@MelonSpeedruns MelonSpeedruns self-requested a review May 26, 2022 15:36
PurpleHato pushed a commit to PurpleHato/Shipwright that referenced this pull request Jun 25, 2022
…-overworld-rendering

and GI to GID for dungeon rewards
Kenix3 pushed a commit to Kenix3/Shipwright that referenced this pull request Oct 19, 2022
* Rumble indefinitely until turned off

* Add rumble cvar

* Register CVar

* Check if controller can rumble to insert rumble pak

* Reduce verbosity of checks

* Remove extraneous const_cast

* Once again remove extraneous const_cast

* Add per-controller settings

* Add nice spacing

* Only display controller entry if pad connected

* Const some stuff
Malkierian pushed a commit to Malkierian/Shipwright that referenced this pull request Nov 20, 2023
* Rumble indefinitely until turned off

* Add rumble cvar

* Register CVar

* Check if controller can rumble to insert rumble pak

* Reduce verbosity of checks

* Remove extraneous const_cast

* Once again remove extraneous const_cast

* Add per-controller settings

* Add nice spacing

* Only display controller entry if pad connected

* Const some stuff
Malkierian added a commit to Malkierian/Shipwright that referenced this pull request Nov 20, 2023
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.

cvar(s) for rumble Rumble Pack not detected but rumbling works
7 participants