Multiplayer Problems: Removing an entity in a HashMap sets the other one to null

Troubleshooting
1

Hello everyone,
it’s me again. :wink:
I’ve managed to add my entities into multiplayer, they’re stored in a HashMap on the server. When the server gets a certain message from the client, it sets the boolean dead on an entity to true.
The entity is then removed in the next update loop.
Now, if that happens, the enemy which is also in the entity list gets replaced by a null (thats what the console output says if I let it print out whats in the list). If I debug it and watch the variables, I understand less than I did before:

Entity_List_after_Removal.png1920×1030 80.1 KB

Here is the update method in my server:

public void updateEntities(float tpf, Server server, BulletAppState bulletAppState, Node rootNode)
{
    for(int i = 0; i < entities.size(); i++)
    {
        System.out.println(entities.size());
        System.out.println(entities.get(i));
        if(entities.get(i) instanceof EntityEnemy)
        {
            ((EntityEnemy)entities.get(i)).update(tpf, players, server);
        }
        else
        {
            if(entities.get(i) != null)
            entities.get(i).update(tpf);
        }

        if(entities.containsKey(i))
        {
            if(entities.get(i).isDead())
            {
                entities.get(i).cleanup(bulletAppState, rootNode);
                //If this line is commented out, then nothing is removed, so the error must be here
               EntityBase e = entities.remove(i);

               server.broadcast(new EntityRemovalMessage(i));
               logger.log(Level.INFO, "Successfully removed entity: {0}", e);
           }
        }
    }
}

If you need any additional methods/classes etc., then please let me know.
Can anyone help me with this?

Robbi Blechdose

2

What is ‘entities’ specifically?

If it’s not a multithreaded data structure then I think you are going to have a bad time.

Edit: or we need to see more code.

One thing to remember… when you remove things from a collection that you are index-iterating over then the size will be smaller and you’ll now skip an index on the next pass. It’s not the right way to do it at all really. You should read about Iterators.

…and anyway, calling get(i) a hundred times in the loop is bad form, also.

2 Likes
3

‘entities’ is a HashMap<Integer, EntityBase>,
I don’t know what you mean with “multithreaded data structure”, sorry (I’d be grateful though if you could point me to some sources).
I already fixed that index-issue in another HashMap, I’ll try to do it here, too.
Could that be the cause of my problem?

Edit: Thanks a lot for the answer.

Robbi Blechdose

4

You use a hashmap… which can be sparse. But then you iterate over it one index at a time.

So of course if you remove index 5 then later when you check for index 5 it isn’t going to be there anymore, right?

Say you have items for 0…10… then you remove five. Now you iterate over 0…9 but get(5) will return null… and you won’t even get to the last element anymore.

I think maybe you just want to iterate over everything in the hashmap? In which case, you only care about the keys. Some Java collections tutorials would probably be very helpful…

5

And if you are populating this map directly from the network messages then you definitely want to worry about multithreading because HashMap can get into an endless loop if you don’t protect access.

Or you can just use ConcurrentHashMap… but ultimately you are going to have to think pretty hard about how you deal with threads. Game servers are multithreaded by nature and certainly the network connections are… each connection has its own thread. (at least one.)

6

Well, the HashMap isn’t used directly from the messages,
but it calls a method which changes thing in the map then.
Thank you for your answers again, I’ll try and read up a bit on the topic.

Robbi Blechdose

7

My thoughts would be the fact that if you’re using the integer to mark the entity as a number removing a previous entity from a list will not rearrange the order of the map.

For example if Monsters 1-5 are on the map and I kill monster 4. You will have monsters 1 2 3 and 5 left so map.get(4) will return a null entity.

8

My code looks like this now:

public void updateEntities(float tpf, Server server, BulletAppState bulletAppState, Node rootNode)
{
    for(int i = 0; i < entities.size(); i++)
    {
        System.out.println(entities.size());
        System.out.println(entities.get(i));
        //When Item is removed, 1 object is in list but is null!?
        if(entities.get(i) instanceof EntityEnemy)
        {
            ((EntityEnemy)entities.get(i)).update(tpf, players, server);
        }
        else
        {
        if(entities.get(i) != null)
            entities.get(i).update(tpf);
        }
        
        /**
        if(entities.containsKey(i))
        {
        if(entities.get(i).isDead())
        {
            entities.get(i).cleanup(bulletAppState, rootNode);
            EntityBase e = entities.remove(i);

            server.broadcast(new EntityRemovalMessage(i));
            logger.log(Level.INFO, "Successfully removed entity: {0}", e);
        }
        }
        **/
    }
    
    Iterator<Integer> it = entities.keySet().iterator();
    while(it.hasNext())
    {
        int key = it.next();
        EntityBase value = entities.get(key);
        if(value.isDead())
        {
        it.remove();
        server.broadcast(new EntityRemovalMessage(key));
        logger.log(Level.INFO, "Successfully removed entity: {0}", value);
        }
    }
}

But the error persists.
Any ideas?

Robbi Blechdose

9

As said before, this is your issue.

Let’s say you put 5 items in your map.
entities.put(0, foo);
entities.put(1, foo);
entities.put(2, foo);
entities.put(3, foo);
entities.put(4, foo);

Now you remove item 2:
entities.remove(2);

This loop… which is NOT BASED ON KEYS will access a missing index and not even get to the last one.
for(int i = 0; i < entities.size(); i++)

Effectively, it’s doing:
entities.get(0);
entities.get(1);
entities.get(2);
entities.get(3);

The results:
0, 1, null, 3

Do you see the issue?

If you want to iterate over all of the KEYS in a map then you should iterate over the KEYS and not some unrelated counter that you just happened to use to generate the keys once.

I’m going to try to let you figure it out from here rather than giving you the exact answer because it’s the kind of basic critical thinking that will be needed to solve your next 1000 bugs, too. :slight_smile:

Edit: Hint: javadoc is your friend.

1 Like
10

I found a lot of your posts useful in this thread.

I was curious about this comment about “get(i).”

Are you saying that with “Hashmaps” calling get in a loop is bad, or in general with any Collection?

If in general, I’m curious what the “best practice” would be? I tried to Google, but nothing really came up.

Any advice, or pointers somewhere would be appreciated :slight_smile: .

11

Calling get(key) in a loop is fine… calling it over and over in one loop iteration is just wasteful. It’s fast but not free.

Dereference it once and reuse it.

MyObject foo = map.get(key);

…instead of calling map.get(key) every time you need it, just use foo.

1 Like
12

Thank you very much for the explanation.

I didn’t realize he called it so many times :slight_smile: .

So the bad practice is essentially he’s calling the same thing multiple times, instead of assigning a return value to it, and using that?

From what you’re saying accessing “entities.get(i)” would be more “costly” than using a return value?

Is that true for any instance?

i.e.,

Double w = geometry.getWidth();

and calling w instead of calling geometry.getWidth(); every time?

Or is this specific to something? i.e., Collections/Mapping?

Thanks :slight_smile:

EDIT: I’m checking out “dereference” now, since I saw it used in your comment.

I guess it makes sense that not having to call the object itself, to get the value, and just storing a return value would be “less wasteful” in any instance?

Thanks.

13

That’s almost free… though one would argue typing w is easier than typing geometry.getWidth() every time.

In the case of accessing a value in a collection, it’s going to be more expensive… how much more depends entirely on the data and the collection implementation.

Besides, in his case he isn’t even using ‘i’ in the loop except for the lookup… and it’s already discussed that the loop is just plain broken. So:
for( SomeObject o : entities.values() ) {
}

…then he doesn’t have to call get(key). Sometimes if we add too much unnecessary cruft, we can’t even see where the real issues are anymore.

1 Like
14

Thanks a lot for the information.

Besides, in his case he isn’t even using ‘i’ in the loop except for the lookup… and it’s already discussed that the loop is just plain broken.

Yeah, I understand what you mean, the term I hear a lot is “there are many ways to skin a cat,” but there are ways that can “work” but really aren’t proper, or using it is a “bad practice.”

There’s a ton of little things to consider though, it’s good to learn about them though :slight_smile: .

Not sure if a lot of these little things are discussed in the Oracle Documentation, but I try to read up on it.

Thanks :slight_smile:

15

There are many ways to skin a cat but most people who bring this up are trying to rationalize some choice that otherwise has no logical reasoning. Every way of skinning a cat will have different trade offs that may meet different requirements. (In a code review with a contractor, every suggestion we made was met with “Well, there is more than one way to skin a cat.”… we didn’t ask him back.)

And in this case, it’s not even the skinning itself we are discussing but the methodology surrounding the skinning. So once you’ve decided to use a knife you can either start from the head and work your way down or you can grab a ruler, measure one inch from the head, skin that…putting the knife down between each stroke and picking it back up again. Then getting out the ruler and measuring the next inch, repeating the process.

There are easy ways and there are hard ways… and there are hard ways masquerading as easy ways because “I didn’t know any better”. That’s what we have here.

16

Thanks a lot for all the answers.
I’ve now rewritten the method to use an entity-internal key which always stays the same. Now I don’t even have to use a HashMap, a list is okay.
And, thanks to everyone for the discussion about “many ways to skin a cat”, I’ve found it and particularly your last post @pspeed very interesting.

Robbi Blechdose