[Python] How do I improve this piece of code?

Game Engine Game Engine Support and Discussion
1

Hello, I started this thread in hopes of strengthening my organization skills with programming.
I had a similar previous thread for something like this, and now with little more experience and practice I’ll be posting more of my code for you guys to critique!

Instead of just one piece of code I’ll be posting snippets here and there for feedback.
I’ll always provide context for code in question.

Let’s get started,

I wrote this small function for a camera script I’m working on. I want to limit the zoom distance in and out, the camera’s distance to the player can’t be over “max” and it can’t be less than “min”. The “pivot” is the player and the “cam_pos” is the camera position object (it’s just an empty with the camera as its child). “vel” is the velocity in which the camera is zooming in or out.


def zoom_limit(self, vel, min, max, pivot, cam_pos):
		
	next = vel + cam_pos.worldPosition
		
	dist_vec = pivot.worldPosition - next
		
	if dist_vec.magnitude < min:
		min_vec = dist_vec.copy()
		min_vec.magnitude = min
			
		extra = -dist_vec + min_vec
			
		vel += -extra
			
	elif dist_vec.magnitude > max:
		max_vec = dist_vec.copy()
		max_vec.magnitude = max
			
		extra = dist_vec - max_vec
		
		vel += extra
		
	return vel

The function basically checks if the next position will be too close to the player (less than min) or too far from the player (more than max), with that information it adjusts the velocity so the cam_pos doesn’t go further than it’s supposed to and I return the altered velocity.

The function is used like this:


velocity = self.zoom_limit(velocity, self.min_zoom, self.max_zoom, self.pivot, self.cam_pos)

Problems that I already see but don’t know how to fix (these I want to address first):

  • The code is repeating itself in two if statements. Is there anyway to make it a general if?
  • I think I have too many parameters, anyway to shorten this? (Inside or outside the code)

Here is the full zoom state code for the camera:


def zoom(self):
		
	self.face_player()
		
	cam_vec = self.input.cam_vec
	y_vel = cam_vec.y*self.fix_sens
		
	velocity = self.cam_pos.worldOrientation*mt.Vector((0, y_vel, 0))
		
	if velocity.magnitude:
		velocity = self.zoom_limit(velocity, self.min_zoom, self.max_zoom, self.pivot, self.cam_pos)
		
		self.cam_pos.worldPosition += velocity
		
	if self.input.key_up("zoom"):
		self.state = "look"

Thank you!

2 


def zoom_limit(self, vel, min, max, pivot, cam_pos):
        
    next = vel + cam_pos.worldPosition
        
    dist_vec = pivot.worldPosition - next
    
    vec_copy = dist_vec.copy()

    vec_copy.magnitude = min(max(vec_copy.magnitude, min), max)

    if dist_vec.magnitude < min:

        extra = -dist_vec + vec_copy
            
    elif dist_vec.magnitude > max:

        extra = dist_vec - vec_copy

    vel += extra
        
    return vel

Should work, I think.

3

Awesome, Solarlune. Your code had a few errors but I worked around them and it simplified even further (using painfully simple math facepalm) to this:


def zoom_limit(self, vel, low, high, pivot, cam_pos):
			
	next = vel + cam_pos.worldPosition
	dist_vec = pivot.worldPosition - next
		
	vec_copy = dist_vec.copy()
	vec_copy.magnitude = min(max(vec_copy.magnitude, low), high)
		
	extra = dist_vec - vec_copy

	vel += extra
			
	return vel

Edit: Now I’m looking to see if I can take off some parameters so it looks cleaner… (if that’s possible).

Shortly, I will post another piece of code I’m working on.

4

“Zooming-in” is not the same as “Moving in”.

If you really want to zoom, you should simply modify the lens property on the camera. Doing that, you can reduce the problem to a simpler function:


def zoom_limited(vel, min, max, cam):
    l = cam.lens + vel
    cam.lens = min if l < min else max if l > max else l

You can lower the argument count by omitting objects which are implicit in the operation (camera, in this case), and then use some other method, internally, to get a proper reference. Grouping related arguments into their own struct (like range = (max, min)) is yet another popular strategy, but if your arg count is somewhere between 2 and 4, it’s not necessary.

Naming is also very important, and it seems like both you and SolarLune like type annotations. In certain cases, those can be useful, but not in this case, because it’s pretty clear (from local context) that “something.worldPosition - whatever” would result in a Vector. There’s no need to clutter things with the “vec” prefix/postfix.

5

Cool!

I’ve realized that looking for methods that might help with goals first might be better than just jumping into my own solution.

I simplified the entire thing.

Here’s the zoom state:


def zoom(self):
		
	self.face_player(self.cam_player_vec)
		
	cam_vec = self.input.cam_vec
	vel = cam_vec.y*self.zoom_speed
		
	self.camera.lens = self.zoom_limited(vel, self.min_zoom, self.max_zoom, self.camera)
		
	if self.input.key_up("zoom"):
		self.state = "look"

and the slightly modified zoom_limited:


def zoom_limited(self, vel, min, max, cam):
	l = cam.lens + vel
	return min if l < min else max if l > max else l

Thanks!

You can lower the argument count by omitting objects which are implicit in the operation (camera, in this case), and then use some other method, internally, to get a proper reference

Do you mean loop through game objects in the scene or use the reference from the class itself?

Here’s an attempt I made at a joystick class. It’s very simple since it only has methods for things I need.

For this one I don’t know if anything needs to be simplified.

6

I think your joystick class’s downward hat constant is supposed to be 4, not 3. I think hat constants are bitwise values, which is why diagonals are those directions literally added together (up = 1, right = 2, so up-right = 3) and why the numbers are bit values (1, 2, 4, 8).

You can check for a value by using bitwise operators on the values. For example, if the hat value for a joystick is 3, then you can use “joystick.hatValues & hat_consts[‘right’]” to see if right is being held down.

As for this joystick class, I’m actually having a bit of trouble understanding it - what is the “input” argument of the class?

Also, I think you could abstract out the profile from the class itself - after all, the joystick doesn’t care what profile you assign it, or if there’s a profile at all. I think it should just be concerned with returning the button, hat, and axis presses as you ask for them. I’m sure you know, but you hard-coded the xbox profile into the set_profile function as well.

As for the previous function, you can get the active camera using the scene’s active_camera property.

7

You can just use the camera reference that you have on self. There is also “active_camera” on the scene reference, as pointed out by SolarLune.

Some things to note:

self.camera.lens = self.zoom_limited(vel, self.min_zoom, self.max_zoom, self.camera)

In this case, it would be perfectly appropriate, and more readable to simply do this:

self.zoom_limited(vel)

The omitted arguments can all be referenced from self, and I see no benefit in explicitly passing them in, because the method is naturally tied to the existing context of the camera, or components that it hosts … There might be some benefit in having max and min as arguments with a default value, but if you’re simply using them to pass in values on self, you don’t really need them.

Here’s an attempt I made at a joystick class. It’s very simple since it only has methods for things I need.
http://www.pasteall.org/52431/python

For this one I don’t know if anything needs to be simplified.

Well, it seems like some of it is already influenced by something I wrote in the past. :wink:

At a quick glance: I think that “update_buttons” could be much nicer if sets were used:


buttons = set(...
active_old = set(...
active = set(joy.activeButtons)

# just activated:

active.difference(active_old)

# active:

active.intersection(active_old)

# just released:

active_old.difference(active)

# none:

buttons.difference(active.union(active_old))

No need for all those conditionals, which is a nice property of “mathy code” like this.

8

Well, it seems like some of it is already influenced by something I wrote in the past. :wink:

*heavily influenced actually, haha, I thought it’d be cool to write a similar class for the joystick since I needed one anyway.

For the sets, I tried it out, here’s what I came up with:


def update_buttons(self):
	buttons = set(self.profile['buttons'].values())
	active_old = set(self.active_old)
	active = set(self.input.activeButtons)
		
	statuses = {a : logic.KX_INPUT_JUST_ACTIVATED for a in active.difference(active_old)}
	statuses.update({a : logic.KX_INPUT_ACTIVE for a in active.intersection(active_old)})
	statuses.update({a : logic.KX_INPUT_JUST_RELEASED for a in active_old.difference(active)})
	statuses.update({a : logic.KX_INPUT_NONE for a in buttons.difference(active.union(active_old))})
		
		
	self.button_status = statuses
	self.active_old = active

Here’s another class I was working on, it’s an Action class, so actions have their own object. It’s really a copy of the sensor but in python. I’m still trying to figure out how to manually advance to the next frame since I think frame increments depends on the game frame rate. In other words, I don’t know how many frames in an Action animation is equal to one second.

Here’s the object:

It’d be used like this:



action = Action(obj, "walk", 1, 20)

def main(cont):

     action.play()

     action.frame = 10

     action.blendin = 5

     action.layer = 1

     action.stop()

It’s supposed to change properties without affecting whatever the animation is doing, but there’s a small 1 frame pause when there is a change, which is why I need to figure out how to increment the action manually. The only thing that causes the animation to restart is when the speed is changed to something between 0 and 1 (maybe a blender bug?).

9

So figured out that animations run at 24 frames (by default).

Does anybody know how to access this value via BGE python? I can’t seem to find it in the documentation.
I’m guessing it either doesn’t exist or I can’t find it.

If it doesn’t exist is there any alternative (hacky maybe)?
Funnily enough, you can access the animation fps through the BGE using the bpy module. Is this a valid solution? Would this work in a standalone .exe?

10

No, it wouldn’t work in a standalone EXE. All I could guess is to assume that the animation frame rate is the same as the game’s logic tick rate (target FPS).

11

You can “find” it by using a frame property animation actuator, or by playing an animation of a certain length in Python and working out how long it took to complete

12

Solarlune, I don’t think the logic tick rate represents animation fps.

@Agoose, I meant I couldn’t find the fps in the documentation haha
Thanks Agoose, I’m not sure how I would use the actuator to reference the fps, but I’ll figure it out.

I want to get back on topic.
Any thoughts on this?
http://www.pasteall.org/52450/python
(same link I’ve posted before)

13

Okay, I’ve been working on some player code.

This player has many states and many properties that are specific to the state.

For example, I calculate velocity on the z-axis based on height, so the jump state uses a property that I call jump_height. I leave this in the init state.

It would look something like this (pseudo code):



class Player:
	def __init__(self):
		
		#move
		self.max_speed = 10
		self.min_speed = 0
                
                self.current_velocity = 0
		
		#jump
		self.jump_height = 3
                self.momentum = 0
			
		self.state = move
		
	def move(self):
		
		self.velocity = self.calc_velocity() #uses self.max_speed and self.min_speed
		
                self.current_velocity = self.velocity #transfer through states

		if input(jump):
			self.state = jump
	
	def jump(self):
		
		self.velocity.z = self.calc_jump() # uses jump height

                self.momentum = calc_momentum(self.velocity.z)

This quickly fills up the more states you have.

So I’m wondering if there’s a way to separate state properties so they’re accessible by all the states, but remain named for the state.

So something like this,



class Move:
     max = 10
     min = 0
     velocity = 0

class Jump:
     height = 4
     momentum = 0

class Player:
	def __init__(self):
		self.state = move
		
	def move(self):
		
		self.velocity = self.calc_velocity() #Uses Move.max and Move.min
		
                Move.velocity = self.velocity #transfer through states

		if input(jump):
			self.state = jump
	
	def jump(self):
		
		self.velocity.z = self.calc_jump() # uses Jump.height

                Jump.momentum = calc_momentum(self.velocity.z)

Or, should I split the states into classes themselves and run them in one Player class?

Is there any nice and organized way of having different persistent variables for every state without filling up the init?

I hope this makes sense,
Thanks.

14

It’s interesting - you commented on my Gearend post about Behaviors, and I’m commenting on this post about my Behaviors which do pretty much exactly what you mentioned above, haha. My Behaviors are very similar to what you have there. In my character class I have a handle_behavior() function that performs the logic for objects depending on the behaviors in the list of behaviors currently. In my system, the behaviors are classes as well - containers for behavior-related variables and references to the actual behavior type (i.e. Follow, Escape, Shoot, Aim, etc).

I feel like it’s kind of unnecessary for your system, though, because the Player is exactly one object that pretty much always has the same behaviors and will always need access to the same number of variables. There’s no real need to separate the variables out since they’re all there anyway. In my system, different objects will have different behaviors, so I thought it’d be nice to separate the variables out for each behavior.

Anyway, I’m not sure.

15

I feel like it’s kind of unnecessary for your system, though, because the Player is exactly one object that pretty much always has the same behaviors and will always need access to the same number of variables. There’s no real need to separate the variables out since they’re all there anyway. In my system, different objects will have different behaviors, so I thought it’d be nice to separate the variables out for each behavior.

Right!

But I’m just looking for something that looks better, something that’s easier to read, you know? In many cases my init method gets filled to the brim with persistent variables and properties and it annoying when you have to scroll 500 times before getting to the top of init. What I would really like is something like this:



class Movement(component):
     def __init__(self):
          
          self.state = move

     def move(self):
          velocity = calc_velocity(move.max_speed) # variables that are part of the move state

          if jump:
               self.state = jump

     def jump(self):
           velocity.z = calc_jump(jump.height) # height is a variables part of the jump state
           if land:
                self.state = land


     def land(self):
           damage = calc_damage(jump.height) # uses the height property to calculate the damage even though height is part of the jump state

Or I’m looking for a way to do it like above with the classes, but something I can actually input inside the init method.

So maybe something like this:



class Movement(component):

     def __init__(self):
          
          self['move'] = Move.properties

          self.state = move

     def move(self):

          velocity = calc_velocity(self['move'].max_speed  OR  self['move']['max_speed'])
16

I use components (state machines) to organize various behaviors into manageable chunks. Example: https://github.com/GoranM/nlu/blob/master/examples/platformer/src/Player.py

17

I’m using your library, so I’m doing the same thing (well, trying).

But just one component is very bulky with no clean way to separate it.

For instance, my Movement component includes, stepping (ledge hand, small ledges, steps), climbing (ladders, vines), move(just moving), jumping (jump, fall, land), combat(attack, dodge, block), animations (shouldn’t be in the movement component, but they rely heavily on the player movement, so they need to be “binded”), swimming (over water, underwater), etc, etc.

For all these states in the movement component there are tons variables and properties, so the init method is filled with just setting variables and properties.

I’m wondering if there’s a simple way to have the properties and variables held somewhere else and easily accessed according to their state.
(I’ve shown many examples above).

OR

If it’s okay to just leave all these things in the init method.

18

I doubt that.

There’s no general reason why Combat and Animation need to be states in a Movement component.

It’s likely that those dependencies can be removed, or can otherwise be expressed on a “component-component” level.

For the latter: Components have access to the game object (gobj), which should in turn carry references to all the relevant components. So, if you really need to manipulate/query one from the other, you can.

Although, you should try to keep things as independent as possible.

Also, note that Components can simply manage other components: In my example, AnimControl manipulates the TexAnim component, and that provides a nice structure.

… It’s hard to give specific advice, because I don’t know the nature of the dependencies you’re dealing with, or why you think they’re necessary.

19

Okay, then here’s the situation,

In the movement component, in the jump state I want the player to lift from the ground after his legs push off the ground (in the jump animation). So in this case I need to check for the specific frame the character jumps.

After, the player is in the air there is a transition into the fall state and then when the player hits the ground there is a transition to the landing state. In the landing state, if the player hits the ground too hard the land animation plays, and I need to check when that finishes before it goes back into the move state.

Another example would be when the player is ledge hanging and the animations need to play at a specific time. The way I do ledge hang is I snap the player to the edge and then I displace it up and over the ledge and at this moment I have the animation follow behind (the armature is displaced to the position the player was and climbs up).

I do the exact same thing when the player is climbing ladders except multiple times. The player displaces up one step, and the animation then plays to follow the player.

The reason I can’t seem to find a way to separate the combat from the movement component is because it’s a separate state. When the player goes into combat, it’s separate from the move, jump, climb states which can’t be active at the time.

20

You could use variables to store the current animation and the current frame of the animation. Seems like if the Movement component controls climbing, for example, then it would signal a flag indicating climbing to be true. The Animation component would check that flag, and if it’s true, play the climbing animation and move the player up a rung every time the animation completes. When it’s done, it would set the climbing flag to false…? Your situation’s a bit different, though, in that in a normal climbing situation I think it would just slide the player up and down the ladder’s normal, essentially, and the climbing animation would just play regardless of what’s actually being traversed.

As for the combat, why can’t you just deactivate the move, jump, and climb states? You could just call functions like “handle_combat” or “handle_movement” and just not call the other functions when it’s time for combat…?