Skip to content

Add removeElementData client side#4745

Open
ArranTuna wants to merge 12 commits into
multitheftauto:masterfrom
ArranTuna:removeElementData
Open

Add removeElementData client side#4745
ArranTuna wants to merge 12 commits into
multitheftauto:masterfrom
ArranTuna:removeElementData

Conversation

@ArranTuna
Copy link
Copy Markdown
Collaborator

@ArranTuna ArranTuna commented Mar 7, 2026

Summary

Fixes #594

removeElementData can now be called on the client which actually deletes (rather than just sets to nil or false) an element data. It also sends a new packet type to the server and syncs it with clients.

Motivation

To actually delete element data so that nil or false isn't being unnecessarily synced to future clients.

Test plan

Although I didn't test with 2 PCs I used it on an object then reconnected to verify that new clients are no longer being synced a removed element data.

Edit: tested with 2 PCs and the remote players have the element data removed correctly.

removeElementData can now be called on the client which actually deletes (rather than just sets to nil or false) an element data. It also sends a new packet type to the server and syncs it with clients.
@ArranTuna ArranTuna requested a review from a team as a code owner March 7, 2026 11:24
@FileEX FileEX added the enhancement New feature or request label Mar 9, 2026
@ArranTuna
Copy link
Copy Markdown
Collaborator Author

ArranTuna commented Mar 10, 2026

Tested everything with 2 PCs, fixed that client variable wasn't being sent to onElementDataChange when a client used removeElementData.

Comment thread Client/mods/deathmatch/logic/CStaticFunctionDefinitions.cpp Outdated
Comment thread Server/mods/deathmatch/logic/CRPCFunctions.cpp Outdated
Comment thread Server/mods/deathmatch/logic/CRPCFunctions.cpp Outdated
Comment thread Server/mods/deathmatch/logic/CRPCFunctions.cpp Outdated
Comment thread Client/mods/deathmatch/logic/CStaticFunctionDefinitions.cpp Outdated
Comment thread Client/mods/deathmatch/logic/CStaticFunctionDefinitions.cpp Outdated
@ArranTuna
Copy link
Copy Markdown
Collaborator Author

I've made the requested changes and re-tested it.

Comment thread Client/mods/deathmatch/logic/CStaticFunctionDefinitions.cpp Outdated
FileEX
FileEX previously approved these changes Apr 9, 2026
Comment thread Client/mods/deathmatch/logic/CStaticFunctionDefinitions.cpp Outdated
Comment thread Server/mods/deathmatch/logic/CRPCFunctions.cpp Outdated
Comment thread Server/mods/deathmatch/logic/CRPCFunctions.cpp Outdated
Comment thread Server/mods/deathmatch/logic/CRPCFunctions.cpp Outdated
@ArranTuna
Copy link
Copy Markdown
Collaborator Author

Requested changes have been added and re-tested.

tederis
tederis previously approved these changes Apr 9, 2026
FileEX
FileEX previously approved these changes Apr 9, 2026
Comment on lines 28 to 38
enum eServerRPCFunctions
{
PLAYER_INGAME_NOTICE,
INITIAL_DATA_STREAM,
PLAYER_TARGET,
PLAYER_WEAPON,
KEY_BIND,
CURSOR_EVENT,
REQUEST_STEALTH_KILL,
REMOVE_ELEMENT_DATA_RPC,
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder why there are so few RPCs here. I feel like there are other ways of defining RPCs that perhaps we're missing?

Like surely there should be a regular setElementData RPC here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, an LLM figured it out. It uses PACKET_ID_CUSTOM_DATA, which is simultaneously used to broadcast to all clients.

I think we should consider reusing that packet, if possible.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For example, here:

if (m_Value.ReadFromBitStream(BitStream))
{
return true;
}

We could do something like this:

if (m_Value.ReadFromBitStream(BitStream)) {
  return true;
} else {
  m_bDelete = true;
  return true;
}

Comment on lines +1068 to +1071
bool isSynced;
CLuaArgument* currentVariable = Entity.GetCustomData(name, false, &isSynced);
if (!currentVariable)
return false;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should be able to skip this check if the entity is a local entity, as DeleteCustomData checks if the field already exists: https://sourcegraph.com/r/github.com/multitheftauto/mtasa-blue@7d8fd38976d6e9fc1edf767697cde2e05b937631/-/blob/Client/mods/deathmatch/logic/CClientEntity.cpp?L501-506

Copy link
Copy Markdown
Member

@qaisjp qaisjp left a comment

Choose a reason for hiding this comment

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

Left some feedback about how we can preserve ordering + reuse the same packet, instead of using a new packet.

@qaisjp
Copy link
Copy Markdown
Member

qaisjp commented May 11, 2026

Sorry! I know it can be frustrating to go through multiple rounds of review :(

@ArranTuna ArranTuna dismissed stale reviews from tederis and FileEX via bca549b May 11, 2026 21:21
@ArranTuna
Copy link
Copy Markdown
Collaborator Author

Made it so the new RPC isn't needed, checked for compliance with coding guidelines, and re-tested.

@ArranTuna ArranTuna requested a review from qaisjp May 11, 2026 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client-side removeElementData

4 participants