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

feat(net/client): Add state bag methods #2314

Merged

Conversation

ivanzaida
Copy link
Contributor

The modifications in this pull request introduce two new natives along with supporting methods for state bags.

Goal of this PR

STATE_BAG_HAS_VALUE: This native is designed to eliminate unnecessary deserialization of state bag values. It serves the purpose of determining whether a value exists for a specific key within the state bag without the need for full deserialization.

GET_STATE_BAG_KEYS: This native expands the utility of state bags by enabling the construction of custom models around the state bag without the necessity to manually check each key for existence. This enhancement streamlines the process of working with state bags and facilitates the creation of more efficient and organized models.

This PR applies to the following area(s)

FiveM, RedM, Server,

Successfully tested on

Game builds: 2944

Platforms: Windows, Linux

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

Fixes issues

The modifications in this pull request introduce two new natives along with supporting methods for state bags.

STATE_BAG_HAS_VALUE: This native is designed to eliminate unnecessary deserialization of state bag values. It serves the purpose of determining whether a value exists for a specific key within the state bag without the need for full deserialization.

GET_STATE_BAG_KEYS: This native expands the utility of state bags by enabling the construction of custom models around the state bag without the necessity to manually check each key for existence. This enhancement streamlines the process of working with state bags and facilitates the creation of more efficient and organized models.
@blattersturm
Copy link
Contributor

blattersturm commented Dec 24, 2023

Do you happen to have any more specific use cases for these two APIs, such as what you are trying to accomplish that makes you run into the need for this?

For example:

  1. What are you running into that would hit 'unnecessary deserialization' and where does it become a problem? (i.e. why would you need to check for a value existing without intending on using the value right after, while also being unable to e.g. set a sentinel value if the value is actually expensive to deserialize? would it make more sense to make an API that lets you defer deserialization, like some sort of 'get-raw'?)
  2. What are you trying to make that requires enumeration of all state values on an entity, including ones that aren't necessarily from your own resource? 'Streamlines the process' doesn't seem like a full explanation here.

I'm not too sure about the API design here either, most other state getters/setters are exposed more naturally in ScRTs, and aren't separate functions to be used directly. Also, having two separate APIs added in a single commit/PR is a bit undesirable as it might be the case only one of the two would be acceptable, and to some extent, both of them look like a solution looking for a problem at worst, or a case of an XY problem at best. ("I'm trying to accomplish X, I think I can do that by doing Y, so I'll try to solve Y and forget all about X")

@ivanzaida
Copy link
Contributor Author

ivanzaida commented Dec 24, 2023

Thanks for the feedback.
I actually wanted to expose the added methods thru all the ScRTs, but it would be breaking the compatibility, since I'm prety sure some of the script makers already used has as their state bag key and it would be a breaking change for them, so I decided not to add this method to JS/Lua ScRTs.

The ideas behind every feature:

  1. Setting a state bag value if it's not present
const entity = Entity(someHandle)
if (!entity.metadataKey) { // why need to deserialize here, if I only want to know if the value is set?
  entity.metadataKey = {...}
}
  1. The idea behind the GET_STATE_BAG_KEYS was that a state bag is a key/value storage, and It would be nice to expose this kind of method to iterate over all the keys that are present in the statebag. Im not pushing this feature, it's just an improvement of the existing API

Also, I know a bunch of developers moving to FiveM from a different multiplayer, and these changes would make their server's migration easier, since the changes in this PR make working with state bags more similar to their old expirience.

@ivanzaida
Copy link
Contributor Author

There is no problem for me to separate the features into two different PR's, if is required.
Need to consult the docs to see if there is a way to integrate the features into the existing ScRTs Entity API

@thorium-cfx thorium-cfx added the triage Needs a preliminary assessment to determine the urgency and required action label Feb 12, 2024
@FabianTerhorst FabianTerhorst self-assigned this Aug 16, 2024
@FabianTerhorst
Copy link
Contributor

Agree with the concern that it doesn't match the current state bag api, but there is no easy way to adjust the current api to support this without breaking changes or without changing it complete. So the change will be accepted and added until we have a new state bag api iteration.

Copy link
Contributor

@FabianTerhorst FabianTerhorst left a comment

Choose a reason for hiding this comment

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

Tested and works fine 👍

@FabianTerhorst FabianTerhorst added ready-to-merge This PR is enqueued for merging and removed triage Needs a preliminary assessment to determine the urgency and required action labels Aug 16, 2024
@slashkeyvalue
Copy link
Contributor

Wouldn't hasvalue also return true for keys whose values were set to null? that'd be weird.

@FabianTerhorst
Copy link
Contributor

FabianTerhorst commented Aug 16, 2024

Wouldn't hasvalue also return true for keys whose values were set to null? that'd be weird.

Yeah, because null values are also replicated. But there is no way to remove state bag values as well. So filtering nulls seems to make sense. Ok considering that the current system does not allow removing values by key there might be a long list of keys that are not really useful.

@FabianTerhorst FabianTerhorst removed the ready-to-merge This PR is enqueued for merging label Aug 16, 2024
@FabianTerhorst
Copy link
Contributor

With #2717 this change makes sense again, because null values will no longer exists inside the data.

@FabianTerhorst FabianTerhorst added the ready-to-merge This PR is enqueued for merging label Aug 18, 2024
@prikolium-cfx prikolium-cfx merged commit 1ee0c70 into citizenfx:master Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR is enqueued for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants