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

Generating power connections doesn't take Group's position into account #24

Closed elswindle closed 2 years ago

elswindle commented 2 years ago

When generating the power connections for a blueprint with multiple tiled electric grids, it will add Associations to electric poles that are located outside the range of the pole. If you tile the following blueprint string with a snap distance of (42,38), call generate_power_connections and then examine the power connections between the electric poles, you'll notice that there are some connections to groups well outside the power pole range. These extra Associations are in range if the position of Group is not applied. Also happens if you perform blueprint translations (much slower since you have to adjust many more entities each iteration)

bp_str = 0eNqdmt1u4jAQhd8l16Gyx7Ed8yqragU06kaiCQqh2qri3TcpPyplspyTS1ryceKZsY/H/szW20O16+qmz5afWb1pm322/PWZ7evXZrUd/9Z/7KpsmdV99ZblWbN6Gz91q3qbHfOsbl6qv9nSHp/zrGr6uq+r0/NfHz5+N4e3ddUNX7g+ue+HZ1//9IsvRJ7t2v3wVNuMPzWSUp59jMBjfgeRK2Rdvy6qbbXpu3qz2LXb6h4kcgYND1XDD67bQzdK8zEP7lmhO1Si+GmJBfmeToN4UokKCaQSr0EiqUSFlFfI5tC9Vy9TOsrTsJohYi91NwT3639eISby3aImyxpMlwRVV6EhLTleujAhXy+pFDafdQqb0FYtC8um9AQmgKl0rv1wGzLRkHCK27Mydwt1GrS8mSgXmz+rulmcp9R7cPF0RsuTv4VbDc5mv1XnBzFsSHQMnvJJHT+tvkWwODs9zlFDOiYkkiZDosVbilv4FLa8UP1Pqpaa4qksChw8UPB4gQ+5qqZBZJNSx1CFI8K9cmIzXtXoDKUxUhqdpeDhe1geLk9OGLjznHJ4vbEnagSY8OrjcCa8FHmcCZutiDPhiko4s4RjRAQJX4/wKBWGXZYRKL4LweNUsG7NqqavcDzmsTa4fgSPd4F7OSLegZ2bEWjkoRoGrhoh0iaxvgmAerhqHB5vD1eNw+Pt4apxeLw9XEMOD5TH1yAiUHANFUSg4BoqiEBFpulyEivmZ9NFcitWa7r4b4avXbe7tusVuReqBkjzfGzSfWww7DSsbpsD2wiYwAgyOuY/wxMcE75zXhDxC2x/QNSNfWD7AxOYMMs9i0Hcc4icwRX3uLpCyRlciJk4g4swo+EMLsS0VDNHisf78CicZ4ZkOtIzQ9CC9MwQ1JOeGYIG0jNDULZbIGrvKZY85rG2RHpmBFoa0jNDUHaRgaDCQzWMIz0zpK2g2oI/Z4ygIT1pwyGdgbThEDSSNhyClqQNh6CJtOEINBnShkNQS9pwCCqkDYegVPv60meWu7agVgOpYNeN+PiUIvlZvly87stTmNULBweAXo50jVzz2nIa2ea1rtEaw0548fHhijV2Vk8ce3VrZJ6t94itt8ZhhzfXfjiquqDOvZ3R4+VnHSWOQ4zEjb0lIBNH6ZE6S3cGOOY0JZvxE9rSrJO/+xH06ok/2zAQ/ZydvjowxWHb0U4/aaevD0xx6BsxTufQl2ImOGzGO30etWxveYrD7mGcnuWWXR8mOMLms9Pz8Nu9APyy1sBCb2tZYTcqo9Dn/HSPbfnt2luevVfd/uS8SlvEJNGHYUWzcjz+Az5/z8Y=
redruin1 commented 2 years ago

This bug was the result of two issues:

  1. add_power_connection and generate_power_connections performed their calculations on the (local) position attribute and not the global_position attribute, causing translations to be incorrect as described. This is fixed.
  2. The way you construct your blueprint involves overlapping the power poles on top of each other. This means that two power poles occupy the same space with two different sets of connections; when you import, one of them is unable to be placed, meaning only half the connections actually end up in the output blueprint, when what you want is for the connections to merge together on a single power pole.

Number 2 is harder to manage. The quickest fix for you at this point would be to modify your original blueprint such that no power poles are placed on top of each other. This is the most "correct" as you probably don't want two distinct electric poles on the exact same position anyway, but this is the least convenient and does not mimic the way Factorio is played, as when you place power poles on top of each other their connections simply merge onto the existing poles. This method can be used as a "quick fix" for now though, or you can manually iterate over your power poles, remove any that exist on the same position and merge their connections.

Another (better) solution would be to check every overlapping entity to see if they can be merged, though this is complex. It might be relatively straightforward in this case with power poles, but what about entities with directions, or recipes, or anything else? You have to check some attributes are equal to actually ensure the two entities are close enough to duplicates, but you can't check all attributes because (in this power poles case) the connections would be different and therefore prevent you from merging them together. Thus, there needs to be a custom "equality" check between overlapping entities on a per-entity basis, which would be a complex addition with lots of testing. That being said, I think this is the better option of the two and would improve the user's experience. I imagine there could be another optional boolean keyword in EntityList.insert called overlap which would attempt to merge entities together so you can enable this functionality when you need it.

If you want to help, what would be really useful would be figuring out this merging truth-table. Electric poles would be global_position, type, and name equivalence, but not power pole or circuit connections. Directional entities would also have to check direction attribute, assembling machines would also have to check their recipe, etc. What about combinators; placing new combinators on top of old ones would update their conditions/signals as well as their connections if we're going with the "placing entities on top of old ones" analogy.

Addendum: After doing some research, it seems that this truth table is somewhat related to fast_replacable_group in Factorio's prototypes. This seems to mimic the behavior of placing individual entities, but not blueprints; you can replace a single yellow belt entity with a red belt, but you can't place a blueprint of a red belt on top of a yellow belt. Which method should we prefer?

elswindle commented 2 years ago

I think we would prefer the blueprint method. You can easily emulate doing fast replacements by modifying the entity's attributes. It may be somewhat useful to have a separate function that does that though, but not include it in the default entity insert method. In that case, we don't add any entity overlapping another except for the ones you mentioned above. If we want to stay true to Factorio blueprinting, any circuit condition, filter, inventory limit, and recipe is overwritten onto any identical entity (same name, type, global_position).

This includes filters of inserters, splitters, and logistics chests; circuit conditions of any circuit connectable entity; recipes of any type of assembling machine (including chemical plants, etc). Circuit connections are simply added on top of existing connections. None are deleted. I can see cases where the user might want to do this. If you are tiling some blueprint and the conditions/filters of the edges must be different than the inner entities, then this would prove useful.

redruin1 commented 2 years ago

Finally finished, as of version 1.0. I wont be perfect, but it should work 95% of the time.