InvisibleSoldiers wrote:You're wrong! This code is nicer to read than original version. also the number of lines of code was decreased. From 179 to 107.
Readability is subjective, but I personally find it much easier to read a code that is not packed solely to decrease the number of lines. For the same reason paragraphs are important in books. Here are some resource you may find constructive:
InvisibleSoldiers wrote:And the listener isn't necessary if you're sure that nothing can remove the 'prop_dynamic_override' entity, only round_restart can. And EntityDictionary itself registers a simlar callback. Retrieving the same dynamic attributes isn't a argument, it's easy can be decided, I did in haste.
I'm talking about the OnPlayerRunCommand listener. Which is extremely noisy as it is called every frame for every players. If there is 30 players on your server, this is like having 30 tick listeners meaning that yes, every calls matters and what your code does behind the scenes is more than important. Take the following example (should be "active_weapon" but that is not the point I'm trying to make):
Syntax: Select all
if self.weapon is not None:
if self.parachute.parent != self.weapon:
self.parachute.parent = self.weapon
You are effectively calling Player.get_active_weapon 3 times which internally request the m_hActiveWeapon property every times. For what? To save a line? This is redundant. Good practice is to use a local assignments and reuse them. Just like I did in the original code:
Syntax: Select all
weapon = player.active_weapon
if weapon is not None:
parent = parachute.parent
if parent != weapon:
parachute.parent = weapon
The weapon is only resolved once, assigned to your local frame and reused; optimization! You are doing the same with fall_velocity, and couldn't reuse it regardless unless you pass it to your class, etc. Same with the self.parachute. Do it once, then work with it. No need to resolve it dozen of times from your self instance.
InvisibleSoldiers wrote:Subclass parachute entity to player object is very well indeed, because a players owns it and it's private, also functions have been defined in Player class which make it clear because they work only with him objects, and it makes the code more in OOP style as Python was planned.
Do you know that any variable which was defined in __init__ method in a class by your definition is the subclassing too? Because int, str, float are classes.
That is not the point. The point is that you already have a player, use it! There is absolutely no reason to add extra layers. This just make the code more complicated, for no benefits, at the cost of performance. Your code add extra casts, extra instantiations, extra methods resolutions, etc. for no benefits, at again, the cost of performance. Keeping everything locally is much faster, for the obvious reason that you do no have to exit the frame and switch from your callback to your class scope. For the same reason from imports are faster than importing the module. For example, when you do:
Syntax: Select all
import math
...
math.radians(...)
Python has to resolve each functions from the module and has to retrieve it dynamically. While when you do the following:
Syntax: Select all
from math import radians
...
radians(...)
Python don't have any resolution to make as it was already resolved in your global scope. I find it rather ironic that you started this thread with timing data as an argument, to then claim performance is not an argument in favour of saving lines. In conclusion, every calls counts. Every resolution counts. Every instantiation counts. If you use subclasses just to claim your code is "pythonic", at the cost of performance, then you are doing it wrong in my opinion.
Anyways, this thread went way out of scope of what it was originally all about so I'm just gonna leave it here as we will have to agree to disagree.
Happy holidays!