redruin1 / factorio-draftsman

A complete, well-tested, and up-to-date module to manipulate Factorio blueprint strings. Compatible with mods.
MIT License
94 stars 17 forks source link

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

Closed elswindle closed 2 years ago

elswindle commented 2 years ago

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.

redruin1 commented 2 years ago

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:

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]
elswindle commented 2 years ago

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.

redruin1 commented 2 years ago

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.

redruin1 commented 2 years ago

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.