multitheftauto / mtasa-blue

Multi Theft Auto is a game engine that incorporates an extendable network play element into a proprietary commercial single-player game.
https://multitheftauto.com
GNU General Public License v3.0
1.41k stars 437 forks source link

Allow allocating new ID's on server #1835

Open TheNormalnij opened 3 years ago

TheNormalnij commented 3 years ago

API

int model = allocateModel( string type, ... config )

This function will allocate new modelID from the end of modelIInfo array( from 20000 to 0 ) Will return false if modelIInfo array is full

bool success = allocateModelAt( int modelID, string type, ... config )

This function will provide manual ID control.

bool success = freeModelAt( int modelID )

Will destroy all objects used by this ID and deallocate modelID

How to it will works

When resource with new ID's will be started, server will send packed with new registered ID's. If client has conflicts with serverside ID, client should reconnect to this server. When client try connect to the server. It should get packed with all new allocated ID's before game and resources started and allocate new ID's

Pirulax commented 3 years ago

I don't think we should use numeric IDs on the server. How about names instead, like:

bool success = allocateModel(string name,  string type, ... config )

We'd add support for passing in model names(of server allocated IDs) to existing functions. Maybe even add support to client-sided engineRequestModel as well? Eg.:

bool success = engineRequestModel(string name, ....)

In the case of a name clash:

Why?

TheNormalnij commented 3 years ago
  • No ID clashes possible, because we're aren't returning ID's, so no need to kick anyone unless they're out of free IDs

Model names clashes possible

  • Server tried to allocate a client id: server succeeds

Server will silently break something at client, bad idea, i think

  • In the future if the client's free ID limit gets increased the server will be limited to around 5000 unique IDs

Impossible, because server owner manages his own resources

  • Future proofing

But you need replace all current API and sync system for new system. This is too big refactor. My solution is simple.

  • numeric model ID's are kinda unpredictable

It's GTA SA design

Pirulax commented 3 years ago

It's GTA SA design

GTA wasn't designed to have a whole Lua API.

This is too big refactor. My solution is simple.

Indeed it's simpler, but also worse. I dont care if the refactor is "too big" (which it really isn't, because we just need to add support for string modelid, and change the sync logic for peds/vehicles (and whatever types can be allocated)). As for the bitstream part (aka syncing part):

Model names clashes possible

Idk wdym here. They're as stated, yes. But, keep in mind I want to make this future proof, so if once we add unlimited models we'll wont break backwards compat. Also, adding this function, and then deprecating it would be a nightmare.

Pirulax commented 3 years ago
  • Server tried to allocate a client id: server succeeds

Server will silently break something at client, bad idea, i think

Yes, but hacked clients could return true always, thus making the server unable to allocate an ID. Perhaps add a client-side debug script telling that "id %s overwritten by the server".

Pirulax commented 3 years ago

So, what do you think? I'd like to start working on this.

AlexTMjugador commented 3 years ago

I agree that it would be nice that the Lua API for this uses names instead of numeric IDs, to avoid numeric ID clashes between uncooperative resources and making sure that the same model has the same ID in both the client and the server. Of course, at the end of the day the model string should still be converted to a numeric ID that will be used in the GTA: SA modelInfo array; for this, a collision-free hashing method would need to be developed, which can be as simple as using a normal hash function but making sure that no hash is used twice (by using open addressing techniques, treating the hashed model IDs as if they were buckets in a hash table). The biggest drawback I can see is that using ID strings feels a bit strange, because people already are used to numeric IDs because of the client-sided functions, but with proper documentation, education and widespread usage this should not be a problem.

Maybe even add support to client-sided engineRequestModel as well?

I'd support this, as it would make the client-side and server-side functions more similar. IIRC, GTA: SA model IDs already have names somewhere we can use (or at least someone assigned such names in the map editor), so it should be possible to deprecate numeric IDs entirely for every function that uses numeric model IDs, like engineReplaceModel.

I also fancy the collision resolution method that @Pirulax pointed out for client and server requested IDs. I agree that the server should have priority over the client.

Pirulax commented 3 years ago

Thanks for your feedback! Now that I think about it, since we'd break net compatibility anyways*, we might as well think about a good way to sync these ID's. My idea would be to only sync the model name once (when allocated / client joins the server), and assign an ID to it, and later just have a bit prefix indicating whenever the ID is server allocated or native. This way hash collisions wouldn't be an issue.

* AFAIK its fine to not be net compatible. (As 1.6 is not planned to be anyways)

TheNormalnij commented 3 years ago

it should be possible to deprecate numeric IDs entirely for every function that uses numeric model IDs,

Numeric ID's are ultrafast because it direct access to model info. You can still use engineGetModelIDFromName if you want use model names only. I wouldn't make current API slower. Sometimes i need to use few functions for 6000 models and with strings it's be slow. Using hashes will make client network little slower also. I don't want to desync getElementModel function between client and server. Now engineGetModelIDFromName use hardcoded names, we need to use modelInfo array for this stuff. So i think that function engineGetModelNameFromID is useless so we can sync only char eModleType + int ID + int nameHash.

So we can use:

-- Server
int model = allocateModel( string type, string name, ...config )
bool success = allocateModelAt( int modelID, string type, string name, ...config )
-- Client
local model = engineGetModelFromName( "model_name" )

Perhaps add a client-side debug script telling that "id %s overwritten by the server".

We can solve collisions by deallocating client-side models or reallocating them. But i think that it's rare situation . It's can happens if scripter will reach ID limit.

I still think that replacing whole net system to using hashes is fun but useless idea.

I would like other MTA members to take part in the discussion.

Pirulax commented 3 years ago

Numeric ID's are ultrafast because it direct access to model info.

A Lua API call probably takes more time than us resolving the model name -> id conversion :D (A hashtable lookup is around 400-500 ns, you can trust me that a Lua call is way more than that). Also, keep in mind I woudn't be using the model info array for resolving the model names. I'd copy all id, name pairs into the hash table, and thats it (since model ID's and names are static).

We can solve collisions by deallocating client-side models or reallocating them. But i think that it's rare situation . It's can happens if scripter will reach ID limit.

What guarantees that free IDs are the same on all clients?

Using hashes will make client network little slower also.

The sync wouldn't get any slower, we only need a bit to indicate whenever the model is a native, or server allocated one. (When a model is allocated, the server would send out a request packet in model name, server id format, and when you sync the model id, you'd use the server id, and a 1 bit prefix to indicate that it's a server ID.

TheNormalnij commented 3 years ago

What guarantees that free IDs are the same on all clients?

We don't need sync clientside allocated id via engineRequestModel.

You can implement same functionality via Lua API now. Just use setElementData for model name and onClientElementStremedIn to sync custom model.

If you want to use only model names for current Lua API, you can do somethings like this:

eModel = { 
   __index = function( self, key )
       return engineGetModelIDFromName( key )
   end;
}

setmetatable( eModel, eModel )

localPlayer:setModel( eModel.truth )

I think mostly server owners don't want to see dynamic changes in ID's on server. For example, car showroom is very common script on servers. With my system server owner only need call allocateModelAt( 19999, ... ) and add info for model 19999 to car config. This script will good work with database without additional changes. With your system server owner need rewrite DB code, config code, creating code... And this is for almost all scripts that use current static ID's. It's painful for scripters.

So, I think we can provide your string model ID API via additional resource for those users who need it.

Pirulax commented 3 years ago

Internal numeric IDs are fragile. You havent answered my question regarding "do all clients have the same free ids?" Right now, with your solution the process would look like this:

Also, as stated above, if in the future we decide to change something, and it'd require us to remove some ID's or something the scripts would break. With a naming abstraction layer i'd work perfectly.

TheNormalnij commented 3 years ago

You havent answered my question regarding "do all clients have the same free ids?"

MTA doesn't support custom GTA SA IPL and IDE mods, so yes, all clients have same free id's at joining.

  • Send packet to clients requesting an ID
  • ????? Clients might send back different IDs??

Client should use server ID anyway and client don't need send something back

  • Or you wan't to have another giant hardcoded table somewhere that has free IDs in it?

MTA server uses table for valid objects we need update and use it. Anyway we need model manager at server to save some specific config for new models( vehicle friendly names, seats count, handling, some args for objects ).

remove some ID's

I tested all free id's, we don't need remove something.

something the scripts would break

Can you provide any potential scenario? And yes, you want break it now.

Disinterpreter commented 3 years ago

I think allocateModelAt and freeModelAt looks legit for https://github.com/multitheftauto/amx because SAMP had non-default objects, I guess we would make analogs and merging them IDs

AlexTMjugador commented 3 years ago

@Pirulax asked me over Discord to copy my thoughts on #1936 here, so they are centralized in the most relevant issue. Here it goes:

The problem I see with using numeric IDs as-is is that they lure people into thinking they are the simplest solution that keeps backwards compatibility, but on practical terms, when actually requesting custom model IDs in the client and the server, complexity scalates quickly, because 1) each server has to reinvent the wheel to manage this problem, 2) depend on a community resource that does this for them or 3) be ignorant of this problem and wonder why things break.

One may think that, if model IDs could only be requested on the server, then these discrepancies would be solved, but requesting a model for every client just because a single client wants to show a vehicle preview in a car shop, for example, wastes a fair amount of bandwidth. Besides, we would break backwards compatibility anyway, because model requests are clientside right now.

It also makes sense to promote element models to their own entity, as with these changes they would have associated logic and a lifetime that's not necessarily the same as the lifetime of the elements that use them, to name some reasons. Insisting on representing entities with primitive data types is known as the primitive obsession antipattern and usually leads to suboptimal solutions in the long (or even short) run. Also, MTA is reasonably expected to keep serverside things synchronized in a transparent and relatively effortless manner, and this approach breaks this expectation.

For reference and clarity purposes, I made this UML class diagram showing an hypothetical OOP Lua API that I think it'd be nice to implement (with some ideas from @sbx320 and @Pirulax):

Custom model IDs API UML class diagram ![Custom model IDs API UML class diagram](https://user-images.githubusercontent.com/7822554/115562591-eda9ae00-a2b6-11eb-8a15-887b3d8add10.png) PlantUML code of the above diagram: ```puml @startuml "Custom model IDs API" title Custom model IDs API class Element { +setModel(ElementModel model) : void +getModel() : ElementModel -- Rest of methods -- ... } class ElementModel { {static} +fromGameModelId(int id) : Model {static} +getAllModels() : Model [1..*] {static} +request(ElementModelType type, ElementModel parent) : CustomModel -int internalId } note left: This API is shared.\n\ninternalId is only known by MTA C++ code.\n\nWhen a client requests a new model,\nits internalId is the next free one for\nuse with GTA.\n\nWhen a server requests a new model,\nits internalId may be a sequential number,\nand clients should request a new model\n(maybe with a different internalId in each\nclient). If a client runs out of internalIds\nduring this operation, it may 1) be kicked\nor 2) the request may return false\n(preferred).\n\nClients keep a server internal ID -> actual\ninternal ID mapping, so they know how to\ntranslate server IDs in packets and so on\nappropriately to keep sync.\n\nServer databases may store models as a\n(type, ID) pair, where "type" distinguishes\nbetween GTA and custom models, and ID\ncan be an index for a Lua table entry that\ncontains data about that model, like a\nfactory method that calls\nElementModel.request to initialize the\nmodel for the first time.\n\nWhen a ElementModel element gets\ndestroyed, the elements that use it are\nreset to the parent element model or\nanother suitable default. This can happen\nwhen the resource that requested them\nstops or destroyElement() is used. ElementModel "1" --> "0..*" ElementModel : children Element <|.. ElementModel enum ElementModelType { PED VEHICLE OBJECT } ElementModel ..> ElementModelType @enduml ``` Edit: a `getAll` method for ElementModel would not be needed because it'd be yet another element type, and we can use `getElementsByType` already.

If we agree that this is a good API, then we can implement these features with less risk of them being rejected or problematic in the long term. Of course it'd depend on things like #1591, but I think it's a good way to go.