mtasa-blue

mtasa-blue copied to clipboard

This closes #2654. Maybe this PR should also block using a value argument larger than 2147483647, but we already had overflowing values with this function.

did you, anyone found backward compatibility issue?

The only issue I can think of, is resources which are relying on the server-side variant of this function being capped.

As for backwards compatibility issues that break scripts, of course it's possible (depending on the amount of spaghetti code), but I can't think of any reasonable scenario where this change should break your script. Again, unless you have some spaghetti-fied logic.

It's actually better this way, because currently if you wanted to bypass the server limit, you'd have to set the money on the client - which obviously can't be trusted - so we'll be doing people a favor.

I'm fine with this change

MAYBE! for now this feature should be hidden behind some flag

I can't think of a resource that will break with this change. The only thing that was changed is that now, when you pass values higher than 99M, you will actually set values higher than 99M. You could always overflow the value.*

The only thing I could think scripters are doing is checking that the value they are trying to set is never above 99M, because that would mean they are giving players less money.

I forgot to update givePlayerMoney. Also, fun fact, you could already set up more than 99M serverside for a player, using takePlayerMoney with negative values:

run takePlayerMoney(me, -150000000)
run getPlayerMoney(me)

Command results: 150000000 [number]

*Maybe an anti-cheat system that checks if the money of the player is above 99M. Highly unlikely imo.

MAYBE! for now this feature should be hidden behind some flag

I think it's fine, next release is 1.6 and we're being a bit more relaxed on this kind of stuff - anyway this is a really minor change with little to no impact.

We first need to understand the underlying reasons behind this behavior before we change it. I'll try to ask around.

Cazomino:

There's definitely a wrap around bug in SA that causes it to wrap, I don't know where that is but I don't think it was an int32 In some cases like that we restricted it to something sensible which was far less though It might also be a rendering issue in the UI

qaisjp:

i would check what happens if you remove the limit and test it out

woovie:

character limit on cash display?

ryden didn't know.

I'll still ask ccw, but I think cazo is probably right.

We need to triple check that changing this behavior doesn't break money: the actual value, the displayed value, the UI. Everything needs to be 100% consistent across the board.

Since the max value in the GTA display is 999.999.999, then that is what the limit should be, as long as everything works absolutely the same. Going beyond the UI limit means a broken UI (we don't want that). And by going beyond what the game actually supports (whatever data type it actually is), can cause game breaking bugs, which is not what we want either.

This change would obviously also have an effect for those who depend on the current limit to be there. How likely is it to cause (major) issues? Probably not very likely. But we need to understand this, that's why deprecations and upgrade notices exist. It all needs to be documented.

As for why the client version doesn't have the limit, no one knows. Could have been a deliberate design decision, networking related, security related, simply overlooked, whatever. Right now though I don't see the point of supporting more than what the UI can display. If servers need larger values, they'll need a custom UI element and custom store for the money value. The alternative is to rewrite the native renderer and wrap any money related functionality with our own system. Neither of which I don't think is all that wanted at the end of the day (just get a lua resource for that).

I'm fine with changing the cap from 99M to 999M. Do we fix the takePlayerMoney issue? Maybe some servers are using that method in order to bypass the cap.

I won't be capping takePlayerMoney for the moment.