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

Source 2: We may need better representations for entity property values of variable length array types #450

Open
esbengc opened this issue Oct 19, 2023 · 2 comments

Comments

@esbengc
Copy link
Contributor

esbengc commented Oct 19, 2023

From what I can tell, properties of variable length array types (e.g. CNetworkUtlVectorBase and CUtlVector) consist of two things: An underlying array and length field. If the length field contains a value n, then only the first n elements in the underlying array should be considered part of the state. Entries n and above in the underlying array are just garbage data. Currently, this is not reflected anywhere in the entity model of this library. Instead, variable length arrays are treated exactly the same as fixed length arrays. This means that client code that needs to consider only the actual content and filter out the garbage will need to manually read the length field and check whether any given index into the array is beyond the length. This is made even more cumbersome by the fact that the length is not kept in the entity state. Instead one manually has to keep track of it in an update handler attached directly to the array property itself (for example m_pWeaponServices.m_hMyWeapons of CCSPlayerPawn), as this handler will be invoked with the updated length whenever the length changes.

In general, event handlers attached to variable length arrays are currently rather clumsy. They work fine with simple reassignment (i.e. aList[i] = v, since they just result in a single field update, but not with add and remove operations. aList.add(v) results in two separate update events: one that changes the length, and one that assigns the array entry. aList.remove(i) is even worse, as it will both result in a decrement to the length, but also cause a reassign to all array entry from i to the end of the list, as all elements after the removed element are shifted down one position. Would it be worth it to get rid of these individual update events, and instead emit just one update event per list per packet, that contains the entire updated state?

This issue currently causes some actual bugs as well. Basically, any time a player loses one or more weapons, Player.Inventory is at risk of getting out of sync. If the weapon list is at the end of the list, then Player.Inventory is not updated at all, as the only update that is emitted is for the length being decremented (it may be fixed later if the weapon is destroyed or picked up by someone else). If the weapon is not the last in the list, then different weapons may be erroneously be removed from Player.Inventory.

Example: Assume that the inventory currently looks like this: [a, b, c, d]. The player now drops b. That means that the following updates happen

m_hMyWeapons.length = 3 // triggers nothing
m_hMyWeapons[1] = c     // b is overwritten, which causes it to be removed from Player.Inventory. c is already in the inventory and is therefore not added again
m_hMyWeapons[2] = d     // c is overwritten, which causes it to be removed from Player.Inventory. d is already in the inventory and is therefore not added again
// m_hMyWeapons now contains [a, c, d].
// Player.Inventory now contains [a, d]. c has erroneously been removed
@markus-wa
Copy link
Owner

markus-wa commented Nov 19, 2023

Sorry I just realised I never commented here.. Thanks for the report on this @esbengc

I tried to fix the inventory issue (see also #472 & #459) - maybe you can help test that fix?

Regarding the more general issue I'd definitely be open to some kind of solution to imporove this system,
though I haven't had time to think about how such a solution might look like..

PRs are defintely welcome - though if we need to make API changes instead of just internal changes we'd need to consider the impacts of these against the benefits.

@markus-wa
Copy link
Owner

markus-wa commented Nov 20, 2023

The inventory issue is fixed in v4.0.0, but the underlying issue with variable length arrays still persists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants