Potential bug in Universe::GenerateObjectID?

[freeorion]

Potential bug in Universe::GenerateObjectID?

[geoffthemedio]

As is, this doesn't make much sense to me either...

But I think it's supposed to be scanning through the ids, looking for one that isn't being used. That would occur if the gap between two successive ids in the ObjectMap was greater than 1. For example, if it saw ids 10, 11, 12, 15, 16, 17. It's trying to find cases like the gap between 12 and 15, as then it can take the last seen id (12) and add 1 to get a new unused id (13).

I took a look through some old versions, but didn't find when this was added or changed. Much earlier, there was no second-try scanning at all.

[Vezzra]

Provided the ObjectMap is ordered by ID (which I think it is), to achieve the behavior you describe I think all that's needed is an additional line within the loop, after the if block, that sets last_id_seen to it->ID(), so in case the if statement doesn't evaluate to true last_id_seen gets properly updated to the last id seen. I guess whoever wrote that code just forgot that line?

So the function would look like this:

int Universe::GenerateObjectID() {
    if (m_last_allocated_object_id + 1 < MAX_ID)
        return ++m_last_allocated_object_id;

    int last_id_seen = -1; // 0 is the first valid object id
    for (ObjectMap::iterator<> it = m_objects.begin(); it != m_objects.end(); ++it) {
        if (it->ID() - last_id_seen > 1)
            return last_id_seen + 1;
        last_id_seen = it->ID();
    }

    return INVALID_OBJECT_ID; // We're screwed.
}

I guess that should work?

[geoffthemedio]

should probably put some comments in there to explain it...

[Vezzra]

I can't get that to work. Although I managed to get this function to correctly yield unused id numbers in case the object id number space gets exhausted (after adding some more lines, the one I suggested isn't sufficient to fix the thing), there goes something wrong in a subtle way still. The only thing I observed is that when I build a Super Tester Building, the effect never executes - the building remains, but the species isn't converted. So something doesn't work as intended.

I finally realized that reusing unused id numbers this way can't work reliably anyway. For that to work, you'd have to ensure that in the same atomic action that determines an unsued id, this id has to be used for an object, and that this object has to be inserted into the object map, because a subsequent call would yield the same id if that doesn't happen. That can't be done within this function.

Meaning, if I call that function several times because I want to create several objects, and after getting the id's and assigning them to my new objects I insert those objects, the result will be a big mess.

So I decided to just remove that loop entirely, as it can't serve it's proposed purpose anyway. I also thought it might be a good idea to log an error message should the object id number space ever get exhausted (which should be almost impossible).

Commit 1989c61, closing this issue.