[Dominik][0.17.59] Fluid mixing by game.merge_forces

[boskid]

As hinted in viewtopic.php?f=182&t=73793#p446338 and because of "Only one bug per report." rule, here it is:

It is possible to merge two fluid systems into one when two forces are merged into one

1. Open "fluid-mixing-by-merge_forces
   save

   fluid-mixing-by-merge_forces.zip

   (170.86 KiB) Downloaded 115 times

2. Code: Select all

   /c game.merge_forces("A", "B")

3. 

   merge_forces_result.png (117.2 KiB) Viewed 5842 times

[Dominik]

Ok I have no idea how to solve this one. Can we pretend that never happened? kidding, but really have no idea.

[Oktokolo]

Easy to solve:
Don't connect pipes/undergroundpipes when connecting them would mix fluids.

And that should be the behaviour in the remainder of the game too:
Instead of trying to prevent building of adjacent pipes or potentially fluid-mixing undergroundpipes, just don't connect them if doing so would mix fluids. Would enable us to have compact pipe buses...

[BlueTemplar]

Bump :
viewtopic.php?f=6&t=73176

[boskid]

Easy to solve:
Don't connect pipes/undergroundpipes when connecting them would mix fluids.

I think hardness depends on internal data structure and implementation that would implement this, not on "just don't connect pipes". What if save contains only data about entities, not about connection between them?

Ok I have no idea how to solve this one. Can we pretend that never happened? kidding, but really have no idea.

I think best solution is (assuming implementation of entity merging into fluid systems using find-union) when rebuilding fluid system, if there are two entities that may connect, find representative of its set (this represents fluid system) and if they are not compatible (this means: both have fluid assigned but they are different fluids) skip merging of this two entities into one fluid segment. This requires save-file to contain fluidSystemID on every fluid entity (pipe, underground pipe, fluid box). Core idea is to store fluidSystemID to mark that entities that could connect, should be left unconnected. Using find-union, fluid system id could be equal to representative entity unit id.
If there is other set of data stored in save-file, solution may be different. no source access thus i am only guessing

[Oktokolo]

Easy to solve:
Don't connect pipes/undergroundpipes when connecting them would mix fluids.

I think hardness depends on internal data structure and implementation that would implement this, not on "just don't connect pipes". What if save contains only data about entities, not about connection between them?

The save data structure has to contain all needed information to deterministically reproduce any possible game world state. So if pipe connections depend on build order (wich they would obviously do if my proposed solution gets implemented), connectuion data has to be in the save (at least for entities wich would normally connect but do not in the saved game world).

[boskid]

I think issue is not with data available in save as it is really capable of storing unconnected underground pipes. Most of bugs i found recently are because one of pipe segments was "updated" and this consists of removing old pipe segment (immediately recomputes fluid systems) and creating new one with new attributes (sometimes unchanged ones). Because of no hint as to which fluid system it should connect to, it connected to all pipes around and so there are this fluid mixing issues here. If merge_forces is performed as silmple for-loop that updates entities one by one and changes their forces from merge.source to merge.dest, then first entity being "updated" (that is: removed) will trigger undergrounds to connect (fluid system rebuild) but then this entity is force-created with new attributes (that is with new "entity force") and it joins both fluid systems. And this case is hard to solve without fluid system rewrite to support true update without underlying remove-recreate. Simply updating entitiy force may not be enough for some cases of overlapping underground pipes and underground belts of different forces that when merging forces needs to be rebuilt.

In other words: Because of underlying set of operations (only remove-create, no true "update") there are two solutions, both wrong:
- using remove-create to change force. This creates above issue when fluid systems are merged
- changing only entity force. This will work but at cost of not connecting underground pipes that after merge should connect together or that should split because of cutter underground pipe moved from other force.

First solution will create desync possibility because when fluid systems are merged, assembler may loose recipe on load and so game will not be save-load stable
Second solution may create desync possibility because underground pipes may connect using old connections (below: crude-oil to crude-oil and acid to acid), but when someone will load map, all underground pipes will be of same force and so inner two underground pipes will not connect to each other (different fluids) and will not connect to their original outer underground pipes (would violate rule to connect only to first underground pipe in required direction)

interleaving-belts-and-pipes-of-different-forces.png (204.76 KiB) Viewed 5731 times

[Oktokolo]

Because of no hint as to which fluid system it should connect to, it connected to all pipes around and so there are this fluid mixing issues here.

Yes, the problem is that there obviously is a check missing since fluid mixing prevention was added to the game. Joining forces seems to be an overlooked edge case.
Blindly connecting adjacent pipes can't work when fluids may not mix. But as fluid mixing prevention requires information about wich fluid is associated to wich fluid box, all needed information has to already be there. Just add the forgotten check and when connections are not allowed, recreate the entities without connecting them.
That should work for simple step-by-step merges and without having to touch the fluid network simulation code.

Simply updating entitiy force may not be enough for some cases of overlapping underground pipes and underground belts of different forces that when merging forces needs to be rebuilt.

Reconnecting undergrounds to other endpoints is fine as long as it does not violate the fluid mixing prevention rule. Undergrounds connect to the nearest appropriately aligned endpoint if allowed to or do not connect at all. That is the defined and expected behaviour.

In other words: Because of underlying set of operations (only remove-create, no true "update") there are two solutions, both wrong:
- using remove-create to change force. This creates above issue when fluid systems are merged

Remove-create is fine if not blindly connecting stuff that may not be connected. And as long as the order of doing the recreates is deterministic, it should not desync because it is done exactly the same on every copy of the game world.

First solution will create desync possibility because when fluid systems are merged, assembler may loose recipe on load and so game will not be save-load stable

As fluid systems will not change assigned liquids when not creating forbidden connections and assemblers are assigning fluid to a connected network depending on recipe, assembler recipe loss because of suddenly changing fluids can't happen after fixing the blindly-connecting issue.

[boskid]

First solution will create desync possibility because when fluid systems are merged, assembler may loose recipe on load and so game will not be save-load stable

As fluid systems will not change assigned liquids when not creating forbidden connections and assemblers are assigning fluid to a connected network depending on recipe, assembler recipe loss because of suddenly changing fluids can't happen after fixing the blindly-connecting issue.

I think issue is worst than this. Lets label entities from first post from left assembler to right assembler: A(left assembler)-B(pipe)-C(underground pipe)-D(und.pipe)-E(pipe)-F(assembler), where before merge, A,B,C are in first force containing crude oil and D-E-F are in second force containing acid.
If merging happens in order "update C but keep it crude-oil", then it will prevent connecting with D, but issue will be when updating B: "update B but keep it crude-oil". Assuming remove-create model (for now it fells like it), after removing B, entity C (already updated) will change fluid system to acid as it can connect to D and is empty (nothing prevents this connection). Then when creating entity B with crude-oil in mind(or pipe), it should not connect to entity C because it changed fluid into acid.
There are some issues with that solution:
- point of split would move from "und.pipe-und.pipe" to other point
- some pipes would change fluid system and it would be hard to notice where split in fluid systems is.
- this would require some art work for when two pipes in straight line would not connect in the middle.

I think better-worst solution would be to deny

Code: Select all

game.merge_forces("A", "B")

with message "Can't mix fluids" (well, it is already widely implemented that way: when setting recipe, when building pipes, when rotating entities - players are already immune to this, they will understand , lets just ignore mods here [even if this function is most likely useful only for mods]). Some entity creation calls and entity teleports are rejected that way

-- edit:
There is no issue with belts in same scenario because belts may contain different items only issue is that one UB-entrance will change into UB-exit when merging forces. #restorefluidmixing
(lets see where all items that are in underground belt section are transfered when merging forces makes underground section to change endpoints - edit: some items are lost)

[BlueTemplar]

Huh, lost rather than dumped overground ??

[boskid]

Huh, lost rather than dumped overground ??

Yup. Here is my test setup:

ub-merge-forces.png (65.47 KiB) Viewed 5702 times

When flushed before merge, there are 118 iron plates and 118 copper plates. After merge there are 118 iron plates and 46 copper plates, no item spill

[Oktokolo]

Assuming remove-create model (for now it fells like it), after removing B, entity C (already updated) will change fluid system to acid as it can connect to D and is empty (nothing prevents this connection). Then when creating entity B with crude-oil in mind(or pipe), it should not connect to entity C because it changed fluid into acid.

I always assumed, that the devs would use low-level functionality for doing the remove-create without triggering the automatisms the player can't avoid.
The cause of this bug report probably is, that the fluid-mixing prevention automatism does not trigger - strongly hinting on the use of low-level functionality for the remove-create.

[Dominik]

It's funny that people are discussing how the code works without having the code. I would not think of trying that I must say really good job as boskid got it quite right!
So how it works:
There are the fluid systems (FS), every fluidbox (FB) is in one FS, FS has the fluid it is locked to specified. The fluid lock is there as long as there is a reason for it - the FS contains a filter (such as the assembler output) or an actual fluid.
The connections are being saved. But that has been only added very recently, before it was never needed. Stuff just connected correctly on load.
The change force indeed just updates entities one by one. The entities handle that individually, but generally by doing destroy - change force - setup. That is done exactly to trigger all the "automatisms" as they make sure that everything is done correctly (there is a lot going on when removing/adding an entity - connecting or registering to various neighbours and lists etc.).
So here the problem is exactly as boskid said. Some pipe changes force, that splits the FS, disconnects the underground pipe from the filter, making it entirely free to connect to the opposite acid one.
Therefore this can't be solved by any simple fluid compatibility check, but might need changes to the force algorithm.

[boskid]

It's funny that people are discussing how the code works without having the code. I would not think of trying that I must say really good job as boskid got it quite right!

It may look funny but it is just useful skill that helps to discuss about complex code (treat it as black box), not available code or code that is beign designed. That way you may be able to catch design issues before writing any code. Here for me it is not available code so i was inferring about how it is implemented based on artifacts it gives (most likely reason for merging when updating entities was remove-create model with intermediate "automatism"). Even if it would happend i was wrong i would have some insight into what kind of problems i would need to solve.

https://img-9gag-fun.9cache.com/photo/a ... svvp9.webm - viewtopic.php?p=411545#p411545

[Oktokolo]

It's funny that people are discussing how the code works without having the code. I would not think of trying that

Why not. When i am wrong (as in this case) i get corrected and learn more about how it actually works. And when i am right, i am happy about that too.
As a Factorio player i like to think about and discuss how things work.

So how it works:
The change force indeed just updates entities one by one. The entities handle that individually, but generally by doing destroy - change force - setup. That is done exactly to trigger all the "automatisms" as they make sure that everything is done correctly (there is a lot going on when removing/adding an entity - connecting or registering to various neighbours and lists etc.).

Looks like faction change code needs to tell the setup function, wich fluid lock an FS has to have to be allowed to connect to the new entity. The setup function has to honor that and don't connect FBs if fluid locks do not match.
If there is a consistency check on load wich checks connections, that probably has to be updated too.

Beeing able to somehow "prime" newly build pipes to belong to a chosen fluid would actually be nice to have in regular gameplay too, as it would be one way to allow compact pipe buses made of regular pipes without the currently mandatory one-tile gap.

[boskid]

Looks like faction change code needs to tell the setup function, wich fluid lock an FS has to have to be allowed to connect to the new entity. The setup function has to honor that and don't connect FBs if fluid locks do not match.
If there is a consistency check on load wich checks connections, that probably has to be updated too.

Beeing able to somehow "prime" newly build pipes to belong to a chosen fluid would actually be nice to have in regular gameplay too, as it would be one way to allow compact pipe buses made of regular pipes without the currently mandatory one-tile gap.

Changing how setup function works should be avoided here: it would violate SOLID-s SRP - setup function would be polluted with fluid specific variables and would need to check each entity type it is updating as "is it fluid related and should i use this extended interface method that requires extra fluid-lock-or-something-else variable?".

With fluid locks, issue is deeper than it looks like. In current implementation fluid locks are because of existing fluid filters(i assume they are just fluid boxes of assembler that are assigned to specific fluids). Every time network is split in half and "automatism" is triggered, it will recompute that one of pipe branches is now empty-and-unfiltered and so underground disconnected section may become connected. Because of this, remove-create model may not be enough to solve this issue. Better one would be to just change force attribute (as there is no other alternative in this particular case of game.merge_forces) and after all entities are updated, perform global fixing - but i feel there may be some recursions when fixing one entity would require other already fixed to be updated again.

Doing temporary fluid locks directly on entities to which currently updated fluid entity is connected would introduce new issue - this fluid locks when removed, should also trigger "automatism" because of cases where changing force would split some fluid networks:

temporary-fluid-lock-issue.png (102.43 KiB) Viewed 5611 times

In this case two inner underground pipes when doing fluid-lock to acid, when merging would first disconnect from one acid source and not connect to crude-oil but would still be assigned to acid, then when merging second underground pipe in middle by setting temporary fluid filter on other underground pipe in middle second underground pipe should not connect to other crude-oil. Then when removing this temporary fluid-lock, inner two underground pipes would isolate, become empty and not assigned and so they would need to be assigned to crude-oil when removing temporary fluid-lock.

Expanding fluid-lock idea to other issues, there are again problems that would come to solve: for example it would prevent some actions that are allowed using other sequence of actions:
- it would prevent rotation that should be allowed but which changes fluid content of some pipes:

temporary-fluid-lock-issue-2.png (55.1 KiB) Viewed 5611 times

That would be because 3-pipe "L" part would be locked to acid and so fluid mixing detection would kick in and prevent rotation. (ref: 73884)
- it would prevent entity teleportation that should be allowed:

temporary-fluid-lock-issue-3.png (56.99 KiB) Viewed 5611 times

Here 2-pipe "I" would become locked and so moving underground pipe 1 tile up again kick-in fluid mixing detecion and would prevent teleportation (ref: 73840)

These cases are extra annoying: they cannot be solved by doing create-remove ("lets test if dest is viable before removing src entity") because they would enter fluid-mixing when they are legit.
For now best solution i can think of would be to create temporary copy of all fluid entities, check if doing update in question would create fluid-mixing then drop this copy. Based on result changes would be performed (they would not create fluid-mixing) or would be rejected without changing anything in game state. This would solve rotation, teleport and upgrade issues but not issue from this bug report...

They are even more annoying because checking if 3-pipe "L" part for rotation or 2-pipe "I" part for teleport would become not filtered when removing entity in question would need whole fluid-system traversal to find if there are other paths to fluid filter that are not going through entity in question. This would need at least O(n) extra memory to perform DFS or BFS as a way to find if any path to fluid-filter exists that is not going through entity in question. This may be part of solution for rotation and teleportation as it would tell if fluid entities that should connect after rotation/teleportation will become compatible after removal of current entity without actual removal of entity.

Then we go to super extra annoying cases: by doing DFS or BFS to check if fluid boxes in new configuration are compatible, we decide "it will not kick-in fluid mixing so we can remove entity in question", this triggers "automatism", system that was decided to be empty-and-not-assigned may connect by disconnected underground pipe pair and so action action should be aborted:

temporary-fluid-lock-issue-4.png (89.19 KiB) Viewed 5611 times

Here as right underground pipes are disconnected, DFS or BFS would detect that 3-pipe "L" section would become unassigned, on removal of underground pipe if would however assign "L" section to water. (this case however for now is working fine)

edit:

temporary-fluid-lock-issue-5.png (87.99 KiB) Viewed 5609 times

This case is broken: underground to water is disconnected, so inner "I" section is assigned to acid only by underground pipe going to acid assembler. However when trying to teleport underground pipe from middle-bottomleft to middle-upleft (to change connection to crude-oil), it should work but for now fluid mixing prevention kicks in (after removal of UP inner part is assigned to water) and entity is not moved, it is then restored and creates fluid mixing of acid and water

edit:

pipe-teleport-issue.png (92.99 KiB) Viewed 5594 times

This case is also interesting: by teleporting underground pipe to connect to crude-oil, when using DFS/BFS to find other connections to acid there would be none so this teleport should be allowed but then by removing underground pipe from source position there would be new path for acid and so teleport would fail if acid underground pipes (on longer path) would connect

[Dominik]

By funny I mean that I would just not even think it is possible to reasonably discuss a code that you don't see. I guess you must be quite experienced programmer to be doing so well at it.

Teleportation is a big issue too, but that is in the other report. I guess I will add some details to that there.

One note that touches some of the problems you described here. There are these cases when we need to test for some fluid mixing, but by doing that, some blocked connection might get repaired and then it would not be recoverable. Such as the second-to-last picture of yours. But that also happens with all those cases of trying to rotate something or changing a recipe, where, temporarily, the fluid lock is removed. For that I use a scoped lock that prohibits fixing the blocked connections until the operation is finished. Only if the operation was successful, the the FS's are checked for possible repairs.

I am, by all means, trying to avoid searches and look-aheads for any checks. So far successfully. The only search performed is for splitting/merging fluid systems, there it can't be avoided. Checks like the one above are just iterations over the fluidboxes and only in systems that are marked that they contain a blocked connection - which typically never happens.

About those not-connecting pipes - that could be possible and there is a reasonable plan on how to do it and it would be nice, but there is no time.

[Oktokolo]

About those not-connecting pipes - that could be possible and there is a reasonable plan on how to do it and it would be nice, but there is no time.

It looks like this bug is indeed not fully fixable without introducing new bugs wich are also hard to fix as long as the automatism itself is not able to deal with two differently fluid-locked FS touching eachother. So it looks like as if you literally have no other choice than to allow adjacent pipes to be part of different fluid-locked FSes.

I recommend that you follow BlueTemplar's suggestion and adopt Picker Pipe Tools into Vanilla. Then you can explicitly clamp affected pipes (each pipe/undergroundpipe wich changees connections otherwise) at the start of a faction merge. This allows you to not getting bitten by the existing automatisms and step-by-step entity replacing code as both work just fine when dealing with clamped pipes.
You could detect the connection-changing entities by copying a layer, then merging entities factions as usual on temporary layer, then comparing original layer's entities with temporary layer's entities for differing connections. Clamp entities wich changed connections, drop temporary layer and perform actual merge on original layer.

Players who never have seen clamped pipes before will be confused when this happens. So you have to also introduce pipe clamping in a tutorial if not in the NPE.

[Dominik]

What do you mean by clamping?
We have the same number of posts.

[Oktokolo]

What do you mean by clamping?

The linked mod allows replacing of pipes with mod-provided pipes that only connect in some directions. That replacement is called clamping on the mod page. It allows players to route pipes adjacent to eachother without autoconnecting them. The mod does clamping in an instantly intuitive way and adds some other piping-related QoL stuff like network and orphan highlighting too. I recommend testing it so you can be impressed about how flexible and user-friendly piping could become and how easy the unfixable bug would be to fix for good if you would have this mod as part of the core mod.

We have the same number of posts.

I should have one more now.