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

Group position not applied when setting its entities to a blueprint's entities #20

Closed elswindle closed 2 years ago

elswindle commented 2 years ago

If you set the entities of a Group equal to the entities of a Blueprint like so:

g1 = Group('g1')
g1.position = (5,5)
g1.entities = b1.entities

The position (5,5) of the Group is not applied to all entities from b1. If each entity is added individually, the position is applied like so:

for entity in b1.entities:
    g1.append(entity)

This works and the offset is applied properly.

redruin1 commented 2 years ago

This is due to an oversight when setting entities; when you add an entity to an EntityList it associates a "parent" structure that contains it. In the first example when you set g1.entities = b1.entities, the reference is just passed along, meaning the entities in g1 still think the blueprint b1 is their parent (Which is why the translation in g1 has no effect). This can be combatted by doing what you've specified or by setting it to a regular list (which is then properly transcribed into an EntityList):

g1 = Group('g1')
g1.position = (5, 5)
g1.entities = b1.entities.data # data is the underlying regular list

Upon revision I guess a copy would have to be made no matter the circumstance, as it would be impossible for one set of data to have both g1 and b1 as their parent. I'll fix this in a bit, though I'm going to want to also integrate some of this into the test suite.

elswindle commented 2 years ago

Ah, that makes sense. My background is mainly in C/C++ so how Python handles basically everything as references still trips me up sometimes.

I just tried the using data, and it breaks all Associations, saying the connected entity is not in the list. Doing the second method above, the same thing happens. I'm assuming this happens because it is trying to reference the entity in the old blueprint. How should this be handled? You can't guarantee the connection is in the new group/blueprint during the appending process. Just do a connection update after adding all the entities to the new list?

redruin1 commented 2 years ago

The main issue you're running into is because of the way I've implemented Associations. Basically, I got fed up of trying to keep track of a unique ID to associate with each entity, so I eventually decided on using the addresses of each entity, which are of course guaranteed to be unique during runtime. The trouble comes when you copy Entities: If you have an association between A and B, and you copy them to copy_A and copy_B, the Association in copy_A still points to the original B and the Association in copy_B still points to the original A. This is what the ValueError is; its looking for the address of the orginal entity in the list of copied elements, where only the copied version exists. There is an entity in the list with the correct value, but the address is different, so it fails it's search.

The natural solution to this is to also make a copy of the association when copying entities. However, this is a complex and expensive operation, and on large blueprints with many associations would exceed pythons stack depth (at least in that particular implementation). In the case of a single blueprint, the current implementation uses some trickery to avoid this copy and still get the correct result, but obviously in your case you need this functionality.

My inclination for a quick fix would be to try to only establish connections between the final entities you want to export; last time I checked, 1KiB_sector_ROM.py in the examples folder manages to use groups, so you might want to take a look there for bits and pieces. In that case adding Groups to Blueprints worked out (though of course I might have broken something since the last time I checked). I can't give a really solid answer at this point as to why exactly it's happening; I need more time to investigate.

redruin1 commented 2 years ago

This is fixed in 0.9.6. I managed to figure out how to copy these structures "properly". Some parts are rather scuffed, and might require a redesign; a lot of the structure of Groups is rather unintuitive, though that could just be a consequence of them being really complex and the level of flexibility I'm providing to the user. In all honesty, it never occurred to me that people would want to set the entities of one blueprint/group to the entities of another blueprint/group, which is why it took me half a week in order to devise a solution. Keep this in mind when using this feature, as due to my ignorance of it it'll probably still be somewhat broken in some subtle way.

elswindle commented 2 years ago

Great to hear!

I'll give a little context to what I'm trying to do here just so you have a better understanding. I created code that generates a factory layout of "factory cells". These factory cells are small factories of 2-7 assemblers with an input/output train station. Currently it is geared towards science production, so given the desired science production rate, cell inputs/outputs and generate rate, the program will place the cells such that it will minimize a cost function I define. The result of the program is not deterministic, so the layout will change with each run so I wanted a way to stitch a bunch of blueprints together (1 for each type of cell) into a single large blueprint to allow the user to just place the entire factory down.

For a 1ksps factory, it needs to place ~500 cells and since it uses LTN, depots also need to be interspersed and it also needs resource interfaces, which together brings it up to nearly 800 cells. In addition, the connecting railway grid is another blueprint that will be tiled and will double that again. So we're basically looking at ~1600 blueprints that I want stitched together. I never thought out the numbers before, but it looks like this will really stress your code to say the least.

redruin1 commented 2 years ago

Sounds like a good stress test. I've yet to run any performance tests, though I suspect it should be alright if you keep the depth of nested groups down. Groups are definitely the least performant part of the codebase though, due to their complexity. If you do run into significant performance issues, feel free to make a new issue about it, though you might want to wait until I push 0.9.6 (which with any luck will be in a little bit)