Skip to content

minor organisational tweaks to CBulletsyncPacket #4651

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.

Sign up for GitHub

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

Draft
qaisjp wants to merge 7 commits into
base: master
Choose a base branch
from
+34 −43
Draft

minor organisational tweaks to CBulletsyncPacket #4651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6262616
Remove brace-init in favour of copy-init
qaisjp Jan 19, 2026
da0f34b
Dry up ResetDamageData
qaisjp Jan 19, 2026
32fba2c
ask question about ResetDamageData
qaisjp Jan 19, 2026
1663f0d
remove dead code
qaisjp Jan 19, 2026
2f1f7ad
ask some more questions
qaisjp Jan 19, 2026
1915561
Document CPacket more
qaisjp Jan 19, 2026
93fdb48
Remove unnecessary check
qaisjp Jan 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 13 additions & 32 deletions Server/mods/deathmatch/logic/packets/CBulletsyncPacket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "CWeaponNames.h"

CBulletsyncPacket::CBulletsyncPacket(CPlayer* player)
: m_weapon(WEAPONTYPE_UNARMED), m_start(), m_end(), m_order(0), m_damage(0.0f), m_zone(0), m_damaged(INVALID_ELEMENT_ID)
{
m_pSourceElement = player;
}
Expand Down Expand Up @@ -101,52 +100,36 @@ bool CBulletsyncPacket::ReadWeaponAndPositions(NetBitStreamInterface& stream)
return true;
}

// Returns false when there's a validation error.
// Make sure to reset damage data to defaults when that happens.
bool CBulletsyncPacket::ReadOptionalDamage(NetBitStreamInterface& stream)
{
if (!stream.ReadBit())
{
ResetDamageData();
return true;
}

stream.Read(m_damage);
stream.Read(m_zone);
stream.Read(m_damaged);

if (IsNaN(m_damage))
{
ResetDamageData();
return false;
}

if (m_damage < 0.0f || m_damage > MAX_DAMAGE)
{
ResetDamageData();
return false;
}

if (m_zone > MAX_BODY_ZONE)
{
ResetDamageData();
return false;
}

if (m_damaged == 0)
{
ResetDamageData();
return false;
}

// Check that target element exists (if specified)
// Note: m_damaged can be INVALID_ELEMENT_ID when shooting at ground/world
if (m_damaged != INVALID_ELEMENT_ID)
{
CElement* pElement = CElementIDs::GetElement(m_damaged);
if (!pElement)
{
ResetDamageData();
return false;
}
// Element exists
}

Expand All @@ -158,19 +141,10 @@ bool CBulletsyncPacket::Read(NetBitStreamInterface& stream)
if (!m_pSourceElement)
return false;

CPlayer* pPlayer = static_cast<CPlayer*>(m_pSourceElement);
if (pPlayer)
{
Comment on lines -161 to -163
Copy link
Member Author

@qaisjp qaisjp Jan 19, 2026

Choose a reason for hiding this comment

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

See #4651 (comment) for more details on this code being deleted

// Check if player is spawned and alive
if (!pPlayer->IsSpawned() || pPlayer->IsDead())
return false;

// Check player position is reasonable relative to bullet start
const CVector& playerPos = pPlayer->GetPosition();
const float maxShootDistance = 50.0f; // Max distance from player to bullet start

// This check will be done after we read positions
}
// Check if player is spawned and alive
CPlayer* pPlayer = GetSourcePlayer();
if (!pPlayer->IsSpawned() || pPlayer->IsDead())
return false;

if (!ReadWeaponAndPositions(stream))
return false;
Expand Down Expand Up @@ -205,7 +179,13 @@ bool CBulletsyncPacket::Read(NetBitStreamInterface& stream)
return false;

if (!ReadOptionalDamage(stream))
{
// todo: do we really need to reset damage data when we're returning
// false? returning false deletes the packet, and other packets don't
// reset internal data based on validation failures.
ResetDamageData();
Comment on lines +183 to +186
Copy link
Member Author

@qaisjp qaisjp Jan 19, 2026

Choose a reason for hiding this comment

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

I think we could get rid of ResetDamageData but not sure if it was added for a specific reason.

I asked AI to double check whether I am right and it agrees:

Looking at the code, when Read() returns false, the packet is indeed dropped and won't be processed further.

1. Packet is deleted immediately after Read() returns false

In CPacketTranslator.cpp (lines 252-256):

// Attempt to read the content, if we fail, delete the packet again
if (!pTemp->Read(BitStream))
{
    delete pTemp;
    pTemp = NULL;
}

This confirms that when Read() returns false, the packet object is immediately destroyed and not used further.

2. Other packets DON'T reset state on validation failure

Checking similar packet implementations:

None of the other packets reset their member variables before returning false from validation.

Conclusion

The ResetDamageData() call at line 192 is not necessary because:

  1. The packet is deleted immediately after Read() returns false
  2. No other packet implementations reset state on validation failure
  3. The pattern across the codebase is to simply return false and let the packet translator handle cleanup

You could safely remove the ResetDamageData() call to be consistent with other packet implementations.

return false;
}

return true;
}
Expand All @@ -224,6 +204,7 @@ bool CBulletsyncPacket::Write(NetBitStreamInterface& stream) const
if (id == INVALID_ELEMENT_ID)
return false;

// why?
if (id == 0)
return false;

Expand Down
17 changes: 6 additions & 11 deletions Server/mods/deathmatch/logic/packets/CBulletsyncPacket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*****************************************************************************
*
* PROJECT: Multi Theft Auto v1.0
* PROJECT: Multi Theft Auto
* LICENSE: See LICENSE in the top level directory
* FILE: mods/deathmatch/logic/packets/CBulletsyncPacket.h
* PURPOSE: Bullet synchronization packet class
Expand All @@ -9,9 +9,6 @@
*
*****************************************************************************/

#ifndef __CBULLETSYNCPACKET_H
#define __CBULLETSYNCPACKET_H

#pragma once

#include "CPacket.h"
Expand Down Expand Up @@ -48,13 +45,11 @@ class CBulletsyncPacket final : public CPacket
static bool IsValidWeaponId(unsigned char weaponId) noexcept;

public:
eWeaponType m_weapon{};
eWeaponType m_weapon = WEAPONTYPE_UNARMED;
CVector m_start{};
CVector m_end{};
std::uint8_t m_order{};
float m_damage{};
std::uint8_t m_zone{};
ElementID m_damaged{INVALID_ELEMENT_ID};
std::uint8_t m_order = 0;
float m_damage = 0.0f;
std::uint8_t m_zone = 0;
ElementID m_damaged = INVALID_ELEMENT_ID;
};

#endif // __CBULLETSYNCPACKET_H
15 changes: 15 additions & 0 deletions Server/mods/deathmatch/logic/packets/CPacket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,22 @@ class CPacket
virtual ePacketOrdering GetPacketOrdering() const { return PACKET_ORDERING_DEFAULT; }
virtual unsigned long GetFlags() const = 0;

// Overridden when it's an incoming packet.
//
// Incoming packets always have CPlayer* as the source element.
//
// - Examples of incoming-only packets: CPlayerDiagnosticPacket, CCommandPacket
// - Examples of dual packets: CBulletsyncPacket, CVoiceDataPacket
virtual bool Read(NetBitStreamInterface& BitStream) { return false; };

// Overridden when it's an outgoing packet.
//
// Outgoing packets may have any element type as the source element. As of
// 2026-01, CStaticFunctionDefinitions::SetPedStat is the caller of
// SetSourceElement with a non-player source.
//
// - Examples of outgoing-only packets: CPlayerStatsPacket, CDebugEchoPacket
// - Examples of dual packets: CBulletsyncPacket, CVoiceDataPacket
virtual bool Write(NetBitStreamInterface& BitStream) const { return false; };

void SetSourceElement(CElement* pSource) { m_pSourceElement = pSource; };
Expand Down
Loading