Skip to content

Fix #472 setElementHealth in onClientPedDamage can cause crash #3914

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

Merged

Conversation

FileEX
Copy link
Contributor

@FileEX FileEX commented Dec 31, 2024

Fix #472

This is my next attempt to fix this issue.

  • Fixed the crash.
  • Fixed the bug where the ped would still die.

The hook in CRenderer::RenderEverythingBarRoads is necessary because when an entity is frequently re-created, a crash occurs due to m_pRwObject being nullptr. This happens, for example, when shooting a ped with a minigun. In my opinion, this should not cause any synchronization or desynchronization issues, but this will become clear during testing if this PR gets merged.

function handleDamage(attacker,weapon,bodypart,loss)
    setElementHealth(source,1)
end
addEventHandler("onClientPedDamage",root,handleDamage)

I believe this is my last attempt to resolve this issue, as I have run out of ideas for fixing it.

@botder
Copy link
Member

botder commented Dec 31, 2024

I did similiar crash fixes before, but if you're looking for a cleaner solution, using a queue for entity re-creation (caused for example by changing model or respawning) and processing it after the game update, should fix all of these crashes.

@FileEX
Copy link
Contributor Author

FileEX commented Dec 31, 2024

I did similiar crash fixes before, but if you're looking for a cleaner solution, using a queue for entity re-creation (caused for example by changing model or respawning) and processing it after the game update, should fix all of these crashes.

What do you mean by processing it after the game update? These crashes are usually caused by the entity pointer becoming invalid during code execution, so it still needs to be replaced with the correct one using hooks, right?

@botder
Copy link
Member

botder commented Dec 31, 2024

I did similiar crash fixes before, but if you're looking for a cleaner solution, using a queue for entity re-creation (caused for example by changing model or respawning) and processing it after the game update, should fix all of these crashes.

What do you mean by processing it after the game update? These crashes are usually caused by the entity pointer becoming invalid during code execution, so it still needs to be replaced with the correct one using hooks, right?

Usually these crashes are caused by a hook that triggers an event, which then calls to functions that cause the invalidation. My suggestion is to delay whatever actions these "invalidating" functions do after the game update, where no chunk of code holds a pointer to an invalidated entity.

@FileEX
Copy link
Contributor Author

FileEX commented Dec 31, 2024

I did similiar crash fixes before, but if you're looking for a cleaner solution, using a queue for entity re-creation (caused for example by changing model or respawning) and processing it after the game update, should fix all of these crashes.

What do you mean by processing it after the game update? These crashes are usually caused by the entity pointer becoming invalid during code execution, so it still needs to be replaced with the correct one using hooks, right?

Usually these crashes are caused by a hook that triggers an event, which then calls to functions that cause the invalidation. My suggestion is to delay whatever actions these "invalidating" functions do after the game update, where no chunk of code holds a pointer to an invalidated entity.

I understand what you mean now, but I’m not entirely sure how this could be achieved. The event needs to be triggered at the appropriate moment, as it is now, to allow it to be canceled correctly. So, how could this be done while still preserving the ability to cancel the event? Additionally, how would it even be possible to delay the code executed in a event function?

@botder
Copy link
Member

botder commented Dec 31, 2024

I understand what you mean now, but I’m not entirely sure how this could be achieved. The event needs to be triggered at the appropriate moment, as it is now, to allow it to be canceled correctly. So, how could this be done while still preserving the ability to cancel the event? Additionally, how would it even be possible to delay the code executed in a event function?

I didn't talk about the event callback function, but whatever function the user decides to call (setElementHealth that respawns dead peds), that cause this effect: entity invalidation. And I don't mean we should delay setting the health, but rather the ped recreation.

@FileEX
Copy link
Contributor Author

FileEX commented Jan 1, 2025

I understand what you mean now, but I’m not entirely sure how this could be achieved. The event needs to be triggered at the appropriate moment, as it is now, to allow it to be canceled correctly. So, how could this be done while still preserving the ability to cancel the event? Additionally, how would it even be possible to delay the code executed in a event function?

I didn't talk about the event callback function, but whatever function the user decides to call (setElementHealth that respawns dead peds), that cause this effect: entity invalidation. And I don't mean we should delay setting the health, but rather the ped recreation.

Done

@botder
Copy link
Member

botder commented Jan 1, 2025

Did you test the code/crash?

@FileEX
Copy link
Contributor Author

FileEX commented Jan 1, 2025

Did you test the code/crash?

Yeah, multiple times even with minigun.

@botder
Copy link
Member

botder commented Jan 1, 2025

If this works as intended, then we should use this approach for vehicles and other things too.

@FileEX
Copy link
Contributor Author

FileEX commented Jan 1, 2025

If this works as intended, then we should use this approach for vehicles and other things too.

You're right, but since this PR also touches the ped damage logic, it would be better to do it in a separate PR soon, so that if a revert is needed due to potential synchronization issues, it won't undo other changes.

@botder botder merged commit 2d3397d into multitheftauto:master Jan 1, 2025
6 checks passed
MTABot pushed a commit that referenced this pull request Jan 1, 2025
2d3397d Fix crash by delaying ped recreation (PR #3914, Fixes #472)
@botder botder added the bugfix Solution to a bug of any kind label Jan 1, 2025
@botder botder added this to the 1.6.1 milestone Jan 1, 2025
@FileEX FileEX deleted the bugfix/onClientPedDamage-crash2 branch January 1, 2025 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Solution to a bug of any kind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setElementHealth in onClientPedDamage can cause crash
2 participants