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

feat(gamestate/fivem): add a ConVar to disable REQUEST_RAGDOLL_EVENT #2244

Closed

Conversation

AvarianKnight
Copy link
Contributor

@AvarianKnight AvarianKnight commented Oct 17, 2023

This is a follow up to some of the discussion from #2237

This adds a ConVar for server owners to disable ragdoll requests set sv_enableRagdollRequests false.

@AvarianKnight AvarianKnight changed the title feat(gamestate): filter REQUEST_RAGDOLL_EVENT for player peds feat(gamestate/fivem): add a ConVar to disable REQUEST_RAGDOLL_EVENT Oct 17, 2023
@AvarianKnight
Copy link
Contributor Author

AvarianKnight commented Oct 17, 2023

As pointed out by @packfile, the initial implementation which filtered to not allow this for player peds specifically would break game functionality (i.e. stun guns, and possibly other stuff).

Players who disable sv_enableRagdollRequests are expected to handle syncing stun gun shots themselves via weaponDamageEvent, and should know that this could break other base-game functionalities

@goncalobsccosta
Copy link

Hi, @AvarianKnight I think this should not break game functionality, I've tested with stun guns and everything is working fine with my code #2237 (and canceling the event)

@AvarianKnight
Copy link
Contributor Author

AvarianKnight commented Oct 18, 2023

Hi, @AvarianKnight I think this should not break game functionality, I've tested with stun guns and everything is working fine with my code #2237 (and canceling the event)

Stun gun was not working when testing this, are you sure had stun guns working when testing?

@packfile
Copy link
Contributor

packfile commented Oct 18, 2023

https://streamable.com/g6z3nr shows a few cases where this event is triggered in gameplay: ramming peds with a vehicle, using the up-n-atomiser, falling over in rapids, punching & shooting peds, and setting them alight to mention a few

Unsure if disabling this event would have any negative impact on the above, so it's worth this change being tested on those?

@blattersturm
Copy link
Contributor

Is it possible for us to somehow validate expected use cases here? A toggle like this seems risky given the side effects.

@AvarianKnight
Copy link
Contributor Author

AvarianKnight commented Oct 18, 2023

Is it possible for us to somehow validate expected use cases here? A toggle like this seems risky given the side effects.

Expected use case is to block players from exploiting the game event you can see some previous discussion in #2237. The main difference is that this PR doesn't expose the event handler to the end user.

@tens0rfl0w
Copy link
Contributor

I was unable to reproduce the mentioned misuse of this event with normal native invocations within the given ScRTs, did anyone confirm that this game event is triggered when the mentioned behavior appears?

Adding to @packfile's notes:
When testing this with two player-peds the behavior seems different than with networked non-player-peds.

  • Using weapon_stungun or weapon_raypistol on a player-ped doesn't seem to trigger the event at all.
  • Hitting a player-ped with something doesn't make it ragdoll in a default MP setup, so no event.
  • Causing a player-ped to ragdoll when hitting it with a car seems to trigger the event sometimes, but canceling/blocking this event doesn't seem to have any impact on the sync behavior. (The strength of the impact makes the difference here.)

As this only seems to be misused on player-peds and blocking this doesn't seem to change any important behavior (at least from what I have gathered), maybe it works to just disable this for player-peds through a ConVar.
(Though I'm not entirely sure if this event is even triggered when a player-ped ragdolls (sometimes it isn't), so maybe this isn't even gonna fix the mentioned problem.)

Additionally, the video in #2237 (comment) shows a somewhat similar ragdoll-physic like when lightly hitting a player-ped with a vehicle, where this event isn't triggered. (Speculation: ragdoll is triggered by the owning client due to collision with a moving entity?)
Here's a short recording of player-ped behavior when the event is blocked: Video of player-ped event logic

@AvarianKnight
Copy link
Contributor Author

Going to post some previous discussion from the engineering discord:

Tasing a ped you don't own does a WEAPON_DAMAGE_EVENT (Player or Regular)

If you tase a regular ped you go through a weird chain:

  1. Player 2 tases Ped
  2. Player 2 Sends out a WEAPON_DAMAGE_EVENT
  3. For Player 1 the ped is tased instantly, Player 2 has to wait for Player 1 to do a REQUEST_RAGDOLL_EVENT even though they no longer own it
  4. Player 2 sees Ped being tased half a second or more later

If you tase a Player everything happens pretty much instantly.

2023-10-18.17-12-51.mp4

A weird quirk is that it doesn't matter if it gets canceled, at least for WEAPON_TASER, you need to actually handle the weaponDamageEvent to cancel it

@tens0rfl0w
Copy link
Contributor

@CiastekbatakPro's solution blocks too many base-game events where ragdoll-physics are used.

I was able to test the solution provided by @AvarianKnight with the mentioned "tools/cheats" and can confirm that blocking this event fixes the mentioned abuse. (Video down below)

I wasn't able to find any game situation where blocking this event had any impact on the behavior (tasing, SetEnableBoundAnkles, hitting a player with a car, ...) when a player-ped is targeted. (Though the behavior of networked non-player peds is affected by blocking this event, so a proper solution would be to block this event for player-peds only with the added ConVar.)

RAGDOLL_REQUEST_EVENT.mp4

@AvarianKnight
Copy link
Contributor Author

AvarianKnight commented Oct 22, 2023

I would like the opinion of @blattersturm here, as there's a bit of a cross roads.

  1. Users will complain that there is no way to block this abuse by third-party code.
  2. Users will complain that certain parts of expected base game functionality don't work

The user will complain either way, I think that having a specific way to block this abuse is probably the better idea even with the draw backs.

That being said, I think if the user opts-in to this possibly breaking change they should be greeted with a warning message that cannot be disabled so they know (even if they copy paste from somewhere else) that this can possibly break functionality.

@goncalobsccosta
Copy link

goncalobsccosta commented Oct 22, 2023

Hi @AvarianKnight, I think with the PR that I made users have the ability to customize the blocked cases. It wouldn't be just an on/off switch!

Tell me what you think.

@AvarianKnight
Copy link
Contributor Author

AvarianKnight commented Oct 22, 2023

Hi @AvarianKnight, I think with the PR that I made users have the ability to customize the blocked cases. It wouldn't be just an on/off switch!

Tell me what you think.

Newer event handlers aren't supposed to expose the event, only convars.

@blattersturm
Copy link
Contributor

In the worst case scenario since this is very game logic dependent, it might be viable to expose an event anyway.

Alternately, filtering this in client-side code to only act in legitimate use cases (or filter out blatantly illegitimate use cases) might also help.

@tens0rfl0w
Copy link
Contributor

It seems like that in a networked environment this event doesn't matter for the actual ragdoll behavior of player-controlled peds. If the requesting client applies any type of damage to the player ped of the receiving client, the receiving client handles the ragdoll itself due to the damage event, the same for tasing and other cases where game logic invokes ped ragdoll events. I couldn't find any edge case where the receiving client doesn't invoke the ragdoll by itself due to missing context.

The only case where this event matters is when the receiving client has some sort of desync happening and rejects/misses the specific game event that invokes the ragdoll (like damage/collision), but this might even improve sync accuracy in those cases as I personally remember a lot of situations where I ragdolled from a collision with a player-driven car even when the car didn't hit me on my client.

@thorium-cfx thorium-cfx added the triage Needs a preliminary assessment to determine the urgency and required action label Nov 3, 2023
@AvarianKnight
Copy link
Contributor Author

Closing this as this PR didn't resolve the initial issue, likely cases of people abusing this are via weapon_stungun and weapon_stungun_mp which users can block via weaponDamageEvent.

@AvarianKnight
Copy link
Contributor Author

Reopening as @tens0rfl0w mentioned this did fix some cases of abuse

@AvarianKnight AvarianKnight reopened this Mar 21, 2024
@AvarianKnight
Copy link
Contributor Author

This was fixed with 468a8fa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Needs a preliminary assessment to determine the urgency and required action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants