redruin1/factorio-draftsman

SpatialHashMap not updated when assigning Group's entities to an EntityList

elswindle opened this issue · 4 comments

In entities.setter of Group, the _entity_hashmap is not updated when the argument is an EntityList object. This causes anything that would use it to return data, i.e. find_entities_filtered, to return nothing. I'm not sure what your intention with this was, but maybe you want to initialize a new EntityList instead of just assigning _entities to the new list? Basically just make the first elif look for both list and EntityList. This method works. Alternatively, it can be handled the same way that Blueprint does by adding each entity into the hashmap.

This is related to #16 and #20. This is fixed in 0.9.6, for both Group and Blueprint. The new setter code is now:

# entities.setter():
self._entity_hashmap.clear()

if value is None:
    self._entities.clear()
elif isinstance(value, list):
    self._entities = EntityList(self, value)
elif isinstance(value, EntityList):
    # Just don't ask
    self._entities = copy.deepcopy(value, memo={"new_parent": self})
else:
    raise TypeError("'entities' must be an EntityList, list, or None")

self.recalculate_area()

Similarly, EntityLike, Group, and Blueprint now all have working __deepcopy__ methods, so they can all be properly copied as well. EntityList can be deepcopied, though as shown in the code above there's a bunch of extra baggage that comes with this so in practice it's recommended you simply deepcopy the parent object and it should just work™.
These deepcopy methods follow the following logic:

  • EntityLikes can be copied, but Associations to other entities are not preserved (they will still point to the original Entities). I haven't decided whether or not this should throw an InvalidAssociationError or if all associations should be cleared on copy, but for now they still point to the original entities unchanged. If an Entity that exists inside some EntityCollection is copied, the parent of the copied Entity will be set to None, as it does not get added to the EntityCollection. That must be done manually:
    blueprint = Blueprint()
    blueprint.entities.append("wooden-chest")
    assert blueprint.entities[0].parent is blueprint
    # Create the copy
    entity_copy = copy.deepcopy(blueprint.entities[0])
    assert entity_copy.parent is None
    assert entity_copy not in blueprint.entities
    # Add the copy to the blueprint explicitly
    blueprint.entities.append(entity_copy)
  • Groups can be copied, and Associations local to that group (fully contained) will be properly copied as well. Attempting to copy a Group that has associations that point outside of the Group will raise InvalidAssociationError on copy. The parent attribute of any contained Entities will be modified to point to the correct, copied value. If the Group object being copied has a parent of it's own, similar to EntityLike it is set to None:
    blueprint = Blueprint()
    group = Group("group") # also I changed group so that you don't *need* an associated ID
    group.entities.append("wooden-chest", id="chest")
    blueprint.entities.append(group)
    # Assert that the blueprint contains the group
    assert blueprint.entities["group"].parent is blueprint
    # Assert that the group contains the entity
    assert blueprint.entities[("group", "chest")].parent is blueprint.entities["group"]
    
    # Create a copy of that group in blueprint
    group_copy = copy.deepcopy(blueprint.entities["group"])
    # group_copy is identical to group
    assert group_copy.id == "group"
    assert len(group.entities) == 1
    assert isinstance(group.entities[0], Container)
    # Copied subentities correctly point to copied parent
    assert group_copy.entities["chest"].parent is group_copy
    # But group_copy does not point to the original blueprint
    assert group_copy.parent is not blueprint
    assert group_copy.parent is None
    # Nor is it contained within it
    assert group_copy not in blueprint.entities
    # Again, it must be manually added if such a thing is desired
  • Blueprints behave nearly identically to Groups, save for the fact that you don't have to worry about non-local Associations as connections in Blueprints can only be inside that particular blueprint.

Both of the entities setters for Blueprint and Group create a use deepcopy to create a new set of data, as due to the nature of EntityList having two or more that point to the same data is undesirable. This makes

blueprint_or_group.entities = other_blueprint_or_group.entities
# conceptually equivalent to
blueprint_or_group.entity_hashmap.clear()
blueprint_or_group.entities = copy.deepcopy(other_blueprint_or_group.entities)
blueprint_or_group.recalculate_area()

As a reminder, EntityList.append also copies anything passed into it by default unless explicitly set with copy = False:

group = Group()
test_entity = Container("wooden-chest")
group.entities.append(test_entity, copy=False)
assert test_entity is group.entities[0]

Impressive, I appreciate the work and am excited to try and break it in new and exciting ways.

I developed my own workaround with a class based on Group that simply replaced all Associations with one pointing to an entity in the current group. This did require me to give IDs to an entity in the parent blueprint that was either power or circuit connectable prior to copying so I could retrieve the ID from the old entity and create a new association pointing to the same ID within the group. I created the following functions to perform this update which are called at the end of the entities setter:

@Group.entities.setter
def entities(self, value):
    # Copy entities
    # ...

    updateNeighbours()
    updateConnections()

def updateNeighbours(self):
    for entity in self.entities:
        if(entity.power_connectable):
            for idx in range(len(entity.neighbours)):
                neighbour = entity.neighbours[idx]
                na = Association(self.entities[neighbour().id])
                entity.neighbours[idx] = na

def updateConnections(self):
    for entity in self.entities:
        if(entity.circuit_connectable):
            for side in entity.connections:
                for color in entity.connections[side]:
                    for idx in range(len(entity.connections[side][color])):
                        point = entity.connections[side][color][idx]
                        connection = point["entity_id"]
                        na = Association(self.entities[connection().id])
                        entity.connections[side][color][idx]["entity_id"] = na

Getting this to work required you to add the group the blueprint first, and then set entities for the group to the entities of the blueprint:

bp_old = Blueprint(bp_string)
id = 0
for entity in bp_old.entities:
    entity.id = str(id)
    id += 1
g = Group('g1')
bp_new = Blueprint()
bp_new.append(g)
bp.entities['g1'].entities = bp_old.entities

This is all a bit hacky, but it worked. Take what you will from this, but just wanted to share this.

I developed my own workaround with a class based on Group that simply replaced all Associations with one pointing to an entity in the current group

This is almost exactly what the current algorithm does, except it uses the id() of each object (so you don't have to manually set it yourself) using the deepcopy metamethod. I'm glad that you managed to find a working solution; this patch should be out either later today or tomorrow assuming I can squash some warnings being issued incorrectly.

Alright, all of your issues should be resolved if you update to version 0.9.6. For the sake of brevity I'm going to close these issues, but if you still have troubles feel free to make a new issue so we have a "clean-slate", so to speak.