Source.Python forums

Perfect PlayerDictionary must have the following properties:
1. It should be one for the whole plugin.
2. It should add and remove player instances when client connect to the server and disconnects from the server respectively.
3. It should if necessary contains functions related to many players.

Also PlayerDictionary must store instances of perfect Player class which have the following properties:
1. It should be inherited from PlayerMixin or another good implementation which also inherited from PlayerMixin.
2. It should store only functions related to the player itself.
3. It shoudn't store functions related to many players.

In this way I do not recommend using standard Player and PlayerDictionary from Source.Python because they don't meet these requirements.
I created a sample prototype of perfect realization of PlayerDictionary and Player.

PlayerDictionary:

Player:

Edit:
Probably better though separate functions related to many players in another class so it doesn't bend the meaning of PlayerDictionary only for associating indexes with some instances.
So we can have a list with player instances for fast iterating over it and PlayerDictionary itself only for retrieving instance by index from it.
I'll redo it later.

I'm honestly not sure why you think this is better than the current implementation.

Removing all the functionality of Entity seems like a horrible idea to me...

Adding get_spectators to the dictionary and having to pass a specific Player to the method doesn't make sense. If you want to deal with 1 specific Player, the method belongs on the Player object.

Why set playerinfo on instantiation? If the playerinfo never gets used, you wasted a call. We already have playerinfo only calling playerinfo_from_index once on the object anyway, because we store that in _playerinfo and if it isn't None, we just return the value we've already retrieved.

Why set permissions on instantiation? Again, if it is never used, you wasted time making that call. Also, permissions can change during a game, so not calling it at runtime could cause you to have old information.

Why remove all doc-strings? They're very helpful and help create our wiki.

Why change view_coordinates to 'get' only? And, why change the code of getting view_coordinates? Is that better than the existing?

The one thing that at least has some merit is adding all exiting players to the dictionary on instantiation. Players are already removed when they exit the server. I can understand why you would want to add all players that way. But, since __missing__ already adds them when you're trying to retrieve one, it would seem to me that that would not be necessary. And, if someone wants to do it themselves, they are more than welcome to it.

I also completely disagree with having 1 dictionary for the entirety of the server. Many scripts inherit from Player and extend its functionality to fit their plugin's specific needs. How is allowing them to create their own dictionary and use their Player class as the 'factory' not the best solution?

satoon101 wrote:I'm honestly not sure why you think this is better than the current implementation.

Read to the end.

satoon101 wrote:Removing all the functionality of Entity seems like a horrible idea to me...

PlayerMixin inhreted from BaseEntity. That's enough. And I just created a class with just the needed functions for my plugin. Maybe I wouldn't have thought of it if Source.Python by default had good implementations and not what we have now.
Why are they bad? Because they have internal cache which if you toggle off through passing caching=False anyway won't do any good because let's see on this
I think everything is clear here without any notes.
Also it doesn't meet the requreiment

InvisibleSoldiers wrote:3. It shouldn't store functions related to many players.

The object must not know about the existence of any other, it is the concern of the other class or function to apply spectators finding.

satoon101 wrote:Adding get_spectators to the dictionary and having to pass a specific Player to the method doesn't make sense. If you want to deal with 1 specific Player, the method belongs on the Player object.

No! It is the concern of the other class or function to apply spectators finding. Player object must not know about the existence of others. This is not logical. Current spectators implementation is sort of hack.

satoon101 wrote:Why set playerinfo on instantiation? If the playerinfo never gets used, you wasted a call. We already have playerinfo only calling playerinfo_from_index once on the object anyway, because we store that in _playerinfo and if it isn't None, we just return the value we've already retrieved.

It's better to check every time on reference to None? And I'm sure it will be used. At least to update player's nickname in database after he was disconnected. I repeat this is a class just for my needs.

satoon101 wrote:Why set permissions on instantiation? Again, if it is never used, you wasted time making that call. Also, permissions can change during a game, so not calling it at runtime could cause you to have old information.

That's what I was afraid. I'll change later.

satoon101 wrote:Why remove all doc-strings? They're very helpful and help create our wiki.

I wasn't going to put it on the wiki, it's my class and just an example of an ideal Player with ideal PlayerDictionary. Ideal because there is no functions related to many players inside a Player class, and PlayerDictionary will be only one for a whole plugin.

satoon101 wrote:Why change view_coordinates to 'get' only? And, why change the code of getting view_coordinates? Is that better than the existing?

Because my need is only get_view_coordinates. And yes, a little better. And again that's my class!

The one thing that at least has some merit is adding all exiting players to the dictionary on instantiation. Players are already removed when they exit the server. I can understand why you would want to add all players that way. But, since __missing__ already adds them when you're trying to retrieve one, it would seem to me that that would not be necessary. And, if someone wants to do it themselves, they are more than welcome to it.

This serves to be taken as an axiom that when player joined, we necessary should add it to the related containers because otherwise the player simply will not exist. For example in spectators counting. We avoid using PlayerGenerator every time, it's really nonsense, we already know our players. We abstract from everything able to give us the opportunity to get a player in some other way than our containers which we update on playerconnect and playerdisconnect. Below I'll show the ideal containers.

satoon101 wrote:I also completely disagree with having 1 dictionary for the entirety of the server. Many scripts inherit from Player and extend its functionality to fit their plugin's specific needs. How is allowing them to create their own dictionary and use their Player class as the 'factory' not the best solution?

I wrote "1 dictionary for the enterity of the plugin", not server, because now in standard Source.Python there is more than 1 using, and it's example of good architercrture usage only 1 PlayerDictionary.

Besides perfect PlayerDictionary, we need the ideal list of players if you need iterating over all players. There are only references, so nothing bad. We will update them both on playerconnect, playerdisconnect, now example:

The addiotinal container is serves only for iterating over it, for example when searching spectators, and playerdict only for taking player instance by index.
I swear you can't think of anything better than this, it's really perfect architecture for each plugin.

I totally agree with everything Satoon said, except that I don't see the merit of having PlayerDictionary automatically syncing itself at all. I mean, this is a container, that you are trying to use as a generator. A container stores objects, while a generator generates them. They both have clearly distinct roles.

InvisibleSoldiers wrote:No! It is the concern of the other class or function to apply spectators finding. Player object must not know about the existence of others. This is not logical. Current spectators implementation is sort of hack.

This makes no sense whatsoever. You want to get players that are spectating a specific player, meaning that the logical implementation is to have that feature directly on that player. Without mentioning that building a list to return instead of yielding the players as you find them is more than redundant. I do however understand that the current implementation of Player.spectators is not flexible as to what type is being yielded, but I truly believe you are approaching it entirely the wrong way. The logical approach would be like we did for Entity.is_in_solid and allows users to pass any generator they wishes to be used. For example:



Now that, in combination of Satoon's suggestion to PlayerIter means that anyone could simply do:



While "perfection" is subjective, I personally believe you are trying to reinvent the wheel by making it square. If your implementation is suitable for your projects, then go for it. But you cannot claim it to be perfect or better by adding redundancy, removing flexibility, or trying to use a concept as if it was another. Perfect or better would improves everything altogether, which is not the case of what is being suggested. That being said, this will be my last post addressing topics directly or indirectly related to your obsession about the internal caching. One of my last minute new year resolution is to adhere to the "pick your battles philosophy", and after many posts trying to have a constructive discussion by bringing valid arguments I can clearly see this debate to be a waste of time. So yeah, good luck, and happy new year!

Okay I'm a little bent about iterating over all players in method of one player, but one PlayerDictionary for a whole plugin which will be updated on player connect or disconnect alongside list with all players for iterating over, it is awesome. I'm even scared to do benchmark with PlayerGenerator every time vs the list. Also probably need to change check instance on Player and replace it to PlayerMixin in classes like RecipientFIlter, because both have index property.