Index of for loop not iterating correctly

Question

I have a for loop that is supposed to iterate a number of times equal to the length of an array. The loop runs the correct number of times, but the index does not properly increment.

I tried manually incrementing i with i = i + 1, but that did not seem to change anything.

room = [['x','x','x','x','x'],['x','.','.','.','x'],['x','.','.','.','x'],['x','.','.','.','x'],['x','x','x','x','x']]
entities = []

class Entity:
    def __init__(self, x_pos, y_pos):
        self.x_pos = x_pos
        self.y_pos = y_pos

        self.new_x_pos = x_pos
        self.new_y_pos = y_pos
    def Move(self, x_move, y_move):
        self.new_y_pos = self.y_pos + y_move
        self.new_x_pos = self.x_pos + x_move
        if self.CheckCollision(self) is True:
            print("collision")
        if self.CheckCollision(self) is False:
            self.new_x_pos = self.x_pos
            self.new_y_pos = self.y_pos
    def CheckCollision(self, entity1):
        for i in range(len(entities)-1):
            #this loop here. It runs twice, but the value of i is zero for both iterations
            entity = entities[i]
            print(i)
            if entity1.new_y_pos == entity.y_pos:
                if entity1.new_x_pos == entity.x_pos:
                    return True
                    break
            else:
                return False
calipso = Entity(1, 1)
entities.append(calipso)
joseph = Entity(3,2)
entities.append(joseph)
print(entities)
calipso.Move(1,1)

print(calipso.x_pos, calipso.y_pos, sep=' ')

I want i to increment each iteration of the for loop, thus i === 0 for the first iteration and i === 1 for the second iteration. Currently, i === 0 for both iterations and I don't know why.

why do you think it runs twice? ranges are right exclusive. range(1) will only produce i=0 and stop afterwards. remove the -1 from len(entities)-1. — Paritosh Singh
– Paritosh Singh, Commented Jan 9, 2019 at 20:10
The loop itself is only running once, but you are calling CheckCollision() twice because within the method Move(), you call CheckCollision() twice (the two if statements). — Endyd
– Endyd, Commented Jan 9, 2019 at 20:11

Calvin Godfrey · Accepted Answer · 2019-01-09 20:37:59Z

1

There's a couple things wrong with your for loop. First of all, if the y positions match but the x values don't, nothing will happen. Barring this case, however, the for loop will always exit after the first iteration because if the positions coincide, return True is called (and by the way, because of the return True, you don't need break after it). And otherwise, it returns False.

So inside Move, CheckCollision is called twice, once for each if statement, which is why i is printed twice, 0 both times.

To fix it, you would have to have CheckCollision return False outside the for loop, so that it checks ALL the entities to make sure it doesn't collide with anything.

The last thing to consider is that you never check that entity1 and entity don't refer to the same object. There are some cases that would result in an entity colliding with itself! Without completely revising the method itself to be like the other answer, the only option to fix that would be some sort of unique id attached to each entity, but revising the method (as in the other answer) is DEFINITELY the better option.

Also, stylistically, method names for classes should always start with a lower case letter (so your methods should be move, checkCollision, etc.).

EDIT: In this specific case, i will never go to 1 anyway. This is because len(entities) is 2, so the for loop will go from 0 to 1, NOT including the end, so no matter what it will stop after the first iteration. But if you had more entities, you would run into the problem described above.

edited Jan 9, 2019 at 20:37

answered Jan 9, 2019 at 20:12

Calvin Godfrey

2,3891 gold badge16 silver badges28 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

John C. Jameson Over a year ago

thank you so much. I fixed the code as you suggested.

Endyd Over a year ago

Be aware, the code change suggestions in this answer will check for collision with itself, which will always be true. You need to make sure within checkCollision() that you iterate through every entity but skip the self entity.

Calvin Godfrey Over a year ago

@Endyd I actually believe it already takes measures to prevent that. in Move, it updates new_x_pos and new_y_pos, and doesn't set y_pos and x_pos until AFTER it checks collision. Because it calls CheckCollision with self, entity1 is actually self the whole time, and so no collisoin will occur.

Calvin Godfrey Over a year ago

@JohnC.Jameson I'm glad that my suggestions helped you, but Endyd's answer is DEFINITELY better and more idiomatic code. I would definitely take their changes into account.

Calvin Godfrey Over a year ago

@Endyd Ah true, I didn't consider the case of Move(0,0). I'll add that to my answer.

|

Endyd · Accepted Answer · 2019-01-09 20:21:02Z

I've refactored your code so that the checkCollision method works the way I think you meant it. See comments within the code. I changed the name of the methods to start with lower case.

class Entity:
    def __init__(self, x_pos, y_pos):
        self.x_pos = x_pos
        self.y_pos = y_pos
        self.new_x_pos = x_pos
        self.new_y_pos = y_pos

    def move(self, x_move, y_move):
        self.new_x_pos = self.x_pos + x_move
        self.new_y_pos = self.y_pos + y_move
        if self.checkCollision() is True:
            # We don't update x_pos and y_pos here since collision means entity shouldn't move
            print("collision")
            return False
        else: # You need to use ELSE here so that you don't end up calling checkCollision twice
            # if checkCollision returned a False, then we can update the position of entity
            self.x_pos = self.new_x_pos
            self.y_pos = self.new_y_pos
            return True

    def checkCollision(self):
        for entity in entities:
            # Iterate through every entity other than self and see if their position is the same as self's possible new position
            if entity != self and entity.x_pos == self.new_x_pos and entity.y_pos == self.new_y_pos:
                return True
        # Return false if no collision occurs after checking through every entity     
        return False

calipso = Entity(1, 1)
entities.append(calipso)
joseph = Entity(3,2)
entities.append(joseph)

calipso.move(1,1) # successful move
print(calipso.x_pos, calipso.y_pos, sep=' ') # prints 2 2 after succesful move
calipso.move(1,0) # collision with joseph, fails
print(calipso.x_pos, calipso.y_pos, sep=' ') # prints 2 2, calipso did not move

Collectives™ on Stack Overflow

Index of for loop not iterating correctly

2 Answers 2

6 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

6 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related