In some cases AddEntity tries to use already occupied Entity IDs

[AlThePhoenix]

Attached is an addition to the GarrisonHolder component which makes it possible to make entities come with a starting garrison once they are created.

Basically, AddEntity() seems to be unable to check what the last used EntityID on a map is. For example: If a carrier (this is the definition we're going to use for the unit with the StartingGarrison) is placed as the 5th entity on the map and its garrison consists of 4 units, the garrisoned units will use IDs from 6 to 9. This causes a crash if more entities were placed on the map after placing the carrier (place the carrier at ID 5, place something else at ID 6, a house for example -> conflict).

What should happen: Instead, AddEntity() should be able to check what the highest EntityID is on the map that is being loaded and use IDs after that or there should be an alternate function that does this.

[AlThePhoenix]

Tested with Alpha 20

[wraitii]

I'm not entirely sure I get the issue. Are you trying to say this happens when loading a scenario/skirmish map?

There is no bug in the code itself, you should rather wait for the start message and spawn the units then. Maps expect and ID and it indeed fails weirdly if you spawn new entities in the meantime.

We might change maps to not expect an ID at all but I'm not sure if that is desirable.

[Freagarach]

Replying to wraitii:

We might change maps to not expect an ID at all but I'm not sure if that is desirable.

Setting priority and type according to the above message.

(See also ​Phab:D1958 for a fix for this specific usecase.)

[elexis]

We might change maps to not expect an ID at all but I'm not sure if that is desirable.

Not sure either, since it seems at least conceivable to have a case where one entity on the map refers to another entity on the map, without using a child-parent relationship (subnodes in xml).

If there is user input, perhaps even any JS mod input such as the uploaded mode and it results in a crash, I guess the task could be to reject the userinput without crashing, or avoiding the error by recomputing the maximum entity ID (if reusing an existing entity ID was the problem) as AlThePhoenix reports. Looking at the actual crashdump would help, or since that is not uploaded, looking at related entity ID code that could crash to figure out which crash he might have referred to. Or looking at the mod code and trying to reproduce.

As he spoke of AddEntity, I see in ComponentManager.cpp

entity_id_t CComponentManager::AllocateNewEntity(entity_id_t preferredId)
{
        // TODO: ensure this ID hasn't been allocated before
        // (this might occur with broken map files)
        // Trying to actually add two entities with the same id will fail in AddEntitiy
        entity_id_t id = preferredId;

        // Ensure this ID won't be allocated again
        if (id >= m_NextEntityId)
                m_NextEntityId = id+1;
        // TODO: check for overflow

        return id;
}

entity_id_t CComponentManager::AllocateNewEntity()
{
        entity_id_t id = m_NextEntityId++;
        // TODO: check for overflow
        return id;
}

[Freagarach]

Yes, this was exactly where I ran into with my turrets patch. (An entity ID was already used and was tried to be used again (​Phab:D1958#108054).) I circumvented by setting entities when the ownership change occurs, which is nice enough.

Recomputing entity IDs would mean that scripts and such ought to be changed as well. So better to break the map gen and inform the user of a broken map? Someone implementing this (example mod) would then need to take care of this via another way.