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

0007818: Fix "explode" for blowVehicle, add it for client #213

Closed
wants to merge 19 commits into from

Conversation

emre1702
Copy link
Contributor

@emre1702 emre1702 commented Jun 30, 2018

Bugtracker:
https://bugs.mtasa.com/view.php?id=7817

Content of PR:

  1. onVehicleExplode and onClientVehicleExplode will get called once at explosion
  2. add "explode" argument for clientside blowVehicle

Previously:

  1. onVehicleExplode gets called two times if a player is nearby
  2. onClientVehicleExplode doesn't get called when using false for explode argument, but onVehicleExplode does

Worked the whole night for it :/
Tested it, but it could be possible that I forgot to test a scenario.

Fixes #455

@botder botder added bug Something isn't working enhancement New feature or request labels Jun 30, 2018
@@ -80,14 +80,19 @@ void CVehicleRPCs::BlowVehicle(CClientEntity* pSource, NetBitStreamInterface& bi
{
// Read out the vehicle id
unsigned char ucTimeContext;
if (bitStream.Read(ucTimeContext))
bool bExplode;
if (bitStream.Read(ucTimeContext) && bitStream.ReadBit(bExplode))
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, this bit was already being sent from the server, right?


// Call onClientVehicleExplode
// Bonus: This way "onClientVehicleExplode will only get called for serversided vehicles.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we expect clientsided vehicles to trigger onClientVehicleExplode?

Copy link
Member

Choose a reason for hiding this comment

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

I would expect it !

Copy link
Contributor Author

@emre1702 emre1702 Jul 21, 2018

Choose a reason for hiding this comment

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

That is the current way, only wanted to comment that.
Currently onClientVehicleExplode will only get triggered when the server sends the explosion.
You can try it on the current build.
This PR doesn't add onClientVehicleExplode for clientside vehicles (also doesn't remove it, cause the event was never triggered for them).

@qaisjp
Copy link
Contributor

qaisjp commented Jul 20, 2018

This looks really good - well done!

@patrikjuvonen patrikjuvonen changed the title #7818 - Fix "explode" for blowVehicle, add it for client 0007818: Fix "explode" for blowVehicle, add it for client Sep 4, 2018
@botder botder self-assigned this Oct 26, 2018
@botder botder added this to the Backlog milestone Mar 4, 2019
Comment on lines +2667 to 2683
if (pInterface->nType == ENTITY_TYPE_VEHICLE) {
CVehicle* pVehicle = pGameInterface->GetPools()->GetVehicle((DWORD*)pInterface)->pEntity;
if (!pVehicle->GetWillExplode())
return false;
}
pExplosionCreator = pGameInterface->GetPools()->GetEntity((DWORD*)pInterface);
}

if (pExplodingEntityInterface)
{
if (pExplodingEntityInterface->nType == ENTITY_TYPE_VEHICLE)
{
CVehicle* pVehicle = pGameInterface->GetPools()->GetVehicle((DWORD*)pExplodingEntityInterface)->pEntity;
if (!pVehicle->GetWillExplode())
return false;
}
pExplodingEntity = pGameInterface->GetPools()->GetEntity((DWORD*)pExplodingEntityInterface);
Copy link
Contributor

Choose a reason for hiding this comment

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

@saml1er please can you check that I've merged this correctly? the author previously had this which conflicted with your entity pool refactor

Copy link

Choose a reason for hiding this comment

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

It's okay, but you don't need to get the entity again from pools class. GetPools()->GetVehicle() can also return nullptr if the interface pointer is corrupt, so we have to check it.

CEntity* pExplodingEntity = pGameInterface->GetPools()->GetEntity((DWORD*)pExplodingEntityInterface);
if (pExplodingEntity && pExplodingEntityInterface->nType == ENTITY_TYPE_VEHICLE) {
    CVehicle* pVehicle = reinterpret_cast<CVehicle*>(pExplodingEntity);
        if (!pVehicle->GetWillExplode())
            return false;
}

Client/game_sa/CVehicleSA.h Outdated Show resolved Hide resolved
@qaisjp
Copy link
Contributor

qaisjp commented Mar 25, 2020

Not tested the master merge, but it compiles.

Looking at the code, it seems that this PR only stops the blow event from being broadcasted twice, but the game still wants to broadcast it twice.

But why does the game try to broadcast it twice?

@ghost
Copy link

ghost commented Mar 26, 2020

But why does the game try to broadcast it twice?

This is the default game behavior. It looks like a bug. It happens sometimes. I can reproduce it in single-player.

@Woovie
Copy link
Contributor

Woovie commented Oct 16, 2020

Updated to latest master. Is this good for merge?

@qaisjp
Copy link
Contributor

qaisjp commented Oct 16, 2020

I think we need to address this comment: #213 (comment)

@Pirulax
Copy link
Contributor

Pirulax commented Nov 23, 2020

Is it a blocker?
Would be gr8 to merge this, sooner or later. It's been sitting here for quite some time.

@qaisjp qaisjp removed this from the Confirmed Issues milestone Dec 7, 2020
@patrikjuvonen
Copy link
Contributor

Thanks for the pull request. Unfortunately this has since been superseded by #1997. If you wish to implement a silent vehicle blowup, feel free to submit a new pull request with the new refactored version as base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onVehicleExplode triggers twice [server-side]
7 participants