Move player finders to a proper place & optimize them

[noaccessl]

I thought it would be better to move these functions to a specialized place.
There was even a comment about it there.

-- These are totally in the wrong place.

Also, optimized them by using bidirectional mapping between IDs and players for easier fetching and cleanup.

[bloodycop7]

Are you sure this cannot have a client side side effect just like ents.Iterator, invalid entities that even IsValid check doesn't fix

[noaccessl]

Are you sure this cannot have a client side side effect just like ents.Iterator, invalid entities that even IsValid check doesn't fix

Most likely it should never happen as this just clears the certain player from stored data. ents.Iterator invalidates cache entirely, setting recaching up for the next call.

As far as I know, these two hooks shouldn't provide invalid players and only called once (regarding players).

[Cheatoid]

Let's have another one of those again, yay! #2124
I can't think of why would you do this, like this is not optimizing anything, it is just introducing bugs and reliability issues.
Are you also not aware that UniqueID has hash collisions because it is CRC32? Also SteamID == BOT for all bots while their SteamID64 is different, you see where I am going here(no?), add 1 bot, player.GetBySteamID("BOT") would return it, add 1 more bot, it returns the different (latest bot), kick the second bot, and do player.GetBySteamID("BOT"), what do you think is the result? nil... whereas in the original code, it would always return the first bot it would find under the entity list. On top of that, the lengths of your lookup/map tables would forever differ from that point onwards.
This could cause some spectacular headache when a player's entity is briefly removed (e.g. after being killed), or some ID being empty or nil during OnEntityCreated due to its nature... when this happens, player object might become NULL entity, at which point something is going to mess up the lookup table... Why would you not invalidate whole cache on full update?

...

..

---

You want to optimize "player finders"? My advice is to stop doing it.

[noaccessl]

Let's have another one of those again, yay! #2124 I can't think of why would you do this, like this is not optimizing anything, it is just introducing bugs and reliability issues. Are you also not aware that UniqueID has hash collisions because it is CRC32? Also SteamID == BOT for all bots while their SteamID64 is different, you see where I am going here(no?), add 1 bot, player.GetBySteamID("BOT") would return it, add 1 more bot, it returns the different (latest bot), kick the second bot, and do player.GetBySteamID("BOT"), what do you think is the result? nil... whereas in the original code, it would always return the first bot it would find under the entity list. On top of that, the lengths of your lookup/map tables would forever differ from that point onwards. This could cause some spectacular headache when a player's entity is briefly removed (e.g. after being killed), or some ID being empty or nil during OnEntityCreated due to its nature... when this happens, player object might become NULL entity, at which point something is going to mess up the lookup table... Why would you not invalidate whole cache on full update?

...

..

You want to optimize "player finders"? My advice is to stop doing it.

Why such complaining, geeky, cocky manner? You could leave straight theses and arguments without some kind of blight.

What bugs and reliability issues exactly? Please, explain.

UniqueID

Are you also not aware that UniqueID has hash collisions because it is CRC32?

Yes, it's a fact. But I'll leave removing/remaking UniqueID-related functions to Rubat. I've left it more for compatibility and completeness.

Bots

What are constructive use cases of player.GetBySteamID() with bots?

Also SteamID == BOT for all bots while their SteamID64 is different, you see where I am going here(no?), add 1 bot, player.GetBySteamID("BOT") would return it, add 1 more bot, it returns the different (latest bot), kick the second bot, and do player.GetBySteamID("BOT"), what do you think is the result? nil... whereas in the original code, it would always return the first bot it would find under the entity list.

It more looks like a cavil.

OnEntityCreated's pitfalls

On top of that, the lengths of your lookup/map tables would forever differ from that point onwards. This could cause some spectacular headache when a player's entity is briefly removed (e.g. after being killed)

1. Why spectacular headache? It is just removal of values in tables. At just some separate tick, it is nothing to overall performance.
2. Player doesn't get removed after being killed.

or some ID being empty or nil during OnEntityCreated due to its nature... when this happens, player object might become NULL entity, at which point something is going to mess up the lookup table...

1. It cannot happen to the player because their Steam-related information is validated during connection, before the player's entity is created.
2. Player probably will be NULL if you try to do it deliberately, but it won't happen normally.

Why would you not invalidate whole cache on full update?

May be too cumbersome. Using such lookup/map tables is the most forthright and fastest solution here.