Martin-H2 commented 7 years ago

the current way of getting a Unique ID is unreliable:

string getFixedSteamId(CBasePlayer@ pPlayer) {
    if(pPlayer is null or !pPlayer.IsConnected())
        return "";
    string steamId = g_EngineFuncs.GetPlayerAuthId(pPlayer.edict());
    if(steamId == 'STEAM_ID_LAN' or steamId == 'BOT')
        steamId = pPlayer.pev.netname;
    return steamId;
}

requesting:

pPlayer.GetUniqueId() ........ Returns a unique id for reliable identification. It doesn't change over server restarts. (in most cases, this is the STEAM_ID)

How BOTS and LAN players are handled, is not the Programmers concern, nor should he get the option to solve it inefficiently (string comparison)

Martin-H2 commented 7 years ago

sorry i cant EDIT the title of this ticket. you may add: "Player.GetUniqueId()"

fnky commented 7 years ago

It should be added to g_EngineFuncs (similar to g_EngineFuncs.GetPlayerAuthId) to not pollute the CBasePlayer class.

g_EngineFuncs.GetPlayerUniqueId(edict_t@ pEdict)

Martin-H2 commented 7 years ago

Well you call it pollution, i call it object orientation ;) Using a helper class (g_EngineFuncs) to obtain an attribute (uniqueID) from a Player-object ? That's bad OO-design, imo. And knowing its ID is the objects responsibility. But well, the whole API is designed in that ancient C-style / OpenGL style. So that one thing doesn't matter anyway i guess.

Well and some other points:

its very counterintuitive (eg why CEngineFuncs and not CPlayerFuncs ?) you WASTE tons of time finding the right helper class in the docu
it blows up the source code and hardens the code maintance for your plugin coders: player.GetUniqueId() vs. g_EngineFuncs.GetPlayerUniqueId(player.edict()) just ... why ?

And by the way pollution: What really pollutes the entity classes (their APIs), is the fact, that every entity method is implemented in CBaseEntity. Like all methods for Sprites, Players, NPC, other point entities ... are now shared between each other, being unusable and illogical on most of the entities. Flooding the documentation with tons of garbage ... and rendering code completion deficient (if we had one on any development platform - haha)

... Or what may be the .BloodColor() of a multimanager ? -___-

fnky commented 7 years ago

Regarding the OO design, that's a subjective matter. However, I agree they could be on the CPlayerFuncs class as static methods. But I think the reason is that they it is also a class exposed from GoldSrc.

I wouldn't wan't the methods the looks up information about the client within a class describing the Player entity. Those are very distinct from each other.

Martin-H2 commented 7 years ago

And while this statement is completely true (a client and a player-entity being 2 different things), one should on the other hand never design an API from the perspective of internal programming / organizational details. And its not the API-users concern, to distinguish such details nor to search for hidden / far fetched helper classes.

Imo, the player objects from the CBasePlayer interface should represent a "valid and fully connected client". Either being actually playing in the world, or observing the game with no physical representation. But it should be the one and only point / interface to handle those connected players/clients consistently. If it was never designed tho handle more then the clients physical representation in the game world, it's a defective design a priori. In this case a "Client" class would be more appropriate, and getting the clients/players physical representation IF present is optional. Especially in the light of Observer-Mode, this Player concept is inconsistent now. At least, that's my opinion as an software architect.

I am very aware of that shift of responsibility into many, (mostly static) helper classes in the world of C/C++/openGL, since i programmed a lot in this area. I am only seeing the AngelScript layer as an opportunity, for mapping those archaic patterns onto a modern OO-Design, which is easy and efficient to work with. (where efficient means, efficient for those plugin coders / mappers, who are new to the project and/or HL1SDK and/or missed the C-era). As for now, the API only exposes the HL1SDK api, which has an highly outdated, non-OO design (for example having all entity members on CBaseEntity and doing CHECKS, which object "class" we have). And some concepts may not even fit the needs of SC anymore (like the Observer-Player problem mentioned above).

So yea, this is an opportunity. If you seize it (and have the time to) - up to you guys :) Maybe in the upcoming switch to ES6 ...

//EDIT: on a sidenote: the Clients name could also be retrieved over an engine helper function. but its player.pev.netname;

fnky commented 7 years ago

I am only seeing the AngelScript layer as an opportunity, for mapping those archaic patterns onto a modern OO-Design, which is easy and efficient to work with.

I agree that it would make it easier for a user to use the language as it was intended rather than having to jump through hoops to figure out how the API works, that also being the result of lacking documentation from the SC team. However, the current way that AngelScript has been implemented into the SC codebase seems to make this far less trivial to change.

Another point is that the closer the script API is mapped to the backend codebase—even though being archaic as you describe it—makes it easier to understand when reading similar source code either from HL1SDK or Xash to understand how these things were originally implemented to mimic the same behaviour—which in turn could also make debugging easier, as you'd expect the outcome to be 1:1 with the original implementation.

baso88 / SC_AngelScript

[Request / Bug] Add Player.GetUniqueId method #54

requesting: