feat(state-bag): initial implementation of state bag filters

[AvarianKnight]

This is an initial implementation of state bag rejections

This was tested and works, this will reject the state bag change and properly sync the original value back to the calling client.

AddStateBagFilter("key", "", (bagName, key, value) => {
    console.log("requested change", key, value);
    setTimeout(() => {
        console.log("actual value", Player(1).state.key)
    }, 0)
	// false is used to reject changes
	return false;
})

There are a few things that I still need to do

Todos

• [x] Figure out why returning nothing in AddStateBagChangeHandler causes the server to crash (maybe its just on debug)
• [x] Reset value properly if originalValue is none
• [x] Document state bag rejection

[AvarianKnight]

This should be good to review now but I have a few questions about the final implementation.

1. Should the client be able to reject server changes
2. Should the server be able to reject server changes

[blattersturm]

This should be good to review now but I have a few questions about the final implementation.

1. Should the client be able to reject server changes
2. Should the server be able to reject server changes

Both should be 'no' to reduce unintended side effects.

[AvarianKnight]

I'm wondering if it wouldn't be better to have a ADD_STATE_BAG_REJECT_HANDLER or something similar, which would be a server-only handler to run before ADD_STATE_BAG_CHANGE_HANDLER.

The main issue with how I have implemented is that its order dependent and still triggers other change handlers even if its rejected, and there isn't any way to really go back and re-run those changes.

[blattersturm]

_FILTER might be more sensible than _REJECT_HANDLER, though even then a separate function is also a bit ugly. Is there a practical use case where this'd be needed?

[AvarianKnight]

Though even then a separate function is also a bit ugly. Is there a practical use case where this'd be needed?

I'm afraid I'm not sure what you mean here

[thelindat]

Is there a practical use case where this'd be needed?

Rejecting on the change handler seems like it could have some unintended side-effects. Pretending these handlers were added by separate scripts..

local somevar

AddStateBagChangeHandler('key', '', function(bag, key, value)
    -- value is assigned to somevar, state change is assumed to succeed
    somevar = value
    return false
end)

AddStateBagChangeHandler('key', '', function(bag, key, value)
    -- reject the state change
    return true
end)

local somevar2

AddStateBagChangeHandler('key', '', function(bag, key, value)
    -- state was already rejected, but this is triggered anyway
    -- value is assigned to somevar2
    somevar2 = value
    return false
end)

[AvarianKnight]

Is there a practical use case where this'd be needed?

The reason to add another native is because as Linden pointed out, it might lead to unexpected behavior by people who use AddStateBagChangeHandler, explicitly having a function that runs before it makes more sense and won't mess with expected behavior.

If the question was more towards "why is this needed", being able to deny state bags is generally a pretty requested feature and would overall be nice, as you can server-authoritative data. This is particularly useful for when working with garage resources as you can have the vehicles table index as a state bag and use that to save later and reject any clients change to that value.

[AvarianKnight]

Going through and bumping a bunch of random prs is generally frowned upon. They'll get to it when they get to it

[thorium-cfx]

Related to #2257

[nihonium-cfx]

Doesn't this has a potential to cause server's main-thread lags?

Each SBag update would be calling into ScRT blocking main-thread and filtering code can be of arbitrary complexity. Main-thread ticks every 50ms (1000 / 20), calling into ScRT is not cheap, I'd say it's safe to eyeball that even for the most simple filter function and just one of it, it'd take ~0.5ms, now let's say you've got 1000 players and your normal game logic sends updates each frame, now you will have ~500ms worth of filtering checks for main-thread, this will cause tick delays and main-thread hitch warnings.

Of course, this is exaggerated example, but even SBag value update on each frame isn't that unrealistic (say you store car's fuel level there), but the more checks you have, the more updates you have, the more sophisticated the filter is - the more delays it will potentially create for main-thread.

[AvarianKnight]

Doesn't this has a potential to cause server's main-thread lags?

Wouldn't this be the case with calling any blocking code inside of a ScRT? Also filtering functions taking 0.5ms is probably a bad (and unrealistic) example.

[thelindat]

https://github.com/citizenfx/fivem/pull/2257 should cover most cases where filtering would be unnecessarily slow or complex, especially with different sync flags (i.e. owner only, no sync, read only). Not sure how the team intends or intended on making statebags rejectable in the first place, and doing it in the change handler itself might lead to side effects (and doesn't solve performance concerns).