[Dominik] [0.17.62] Fluid mixing when building underground pipe

[boskid]

I know this is modded, but consistency crash is from base game. Pipe prototype consists of 5 fluid box pipe connections and this gives new set of issues. Reproduction:

1. Download advanced-fluid-handling-issue.zip, sync mods and load
   save

   advanced-fluid-handling-issue.zip

   (168.5 KiB) Downloaded 160 times

   

   advanced-underground-piping-issue-1.png (65.19 KiB) Viewed 8114 times

2. Issue 1: when hovering on center "1->4 pipe segment" it shows as disconnected in all 4 directions, when in fact it is only disconnected to underground pipe to the left
3. Pippete this inner "1->4 pipe segment" and build 1 tile above to get this:

   

   advanced-underground-piping-issue-2.png (63.57 KiB) Viewed 8114 times

4. Issue 2: There is fluid mixing after previous step (acid output is connected to pipe that is shown to be crude oil). Consistency fails:

   Code: Select all

   952.479 Error FluidBox.cpp:593: Fluid not compatible with this fluidbox's filter

-- edit: Removed mod part from topic as there is reproduction without "Advanced Underground Piping" mod

[boskid]

Same issue triggered without any mods:

1. Load "74116-fluid-mixing.zip" or build it yourself
   save

   74116-fluid-mixing.zip

   (151.67 KiB) Downloaded 145 times

   

   74116-vanilla-1.png (53.88 KiB) Viewed 8109 times

2. Remove cutter pipe below one lamp

   

   74116-vanilla-2.png (55.49 KiB) Viewed 8109 times

3. Build underground below two lamps to get into this state:

   

   74116-vanilla-3.png (52.91 KiB) Viewed 8109 times

4. Fluid mixing

Reproduction is based on fact that building this one underground pipe in one action disconnects filter from disconnected pair on left (by cutting existing underground pipe pair on right) and connects it again (by providing new path)

[Staplergun]

This mod has been the bane of the fluid system since 0.17 launch lol.

[Dominik]

Nice reproduction in basic game. I think that this is actually the only case where this bug happens (in vanilla).
Might be hard to fix, I think that it is similar to the issues in the previous batch. Will work on that tomorrow.

[boskid]

I tought it will be easy, it feels so similar to this case:

74116-no-bug-with-rotation.gif (106.52 KiB) Viewed 7902 times

Here when rotating pipe IIRC it is solved by (what you called) scope lock that prevents blocked underground pipes to connect in between of remove-create. Is it really different to use similar solution with building new pipe-to-ground?

-- edit:
And this little harder case is also working fine:

74116-no-bug-with-rotation-2.gif (89.81 KiB) Viewed 7892 times

It differs from first case as in first rotation will not change acid fluid system id, and second case will change acid fluid system id

-- edit:

Code: Select all

rotateUndergroundPipe() {
    disable disconnected undergrounds from connecting;

    remove pipe-to-ground in old rotation; // splits FS's
    build pipe-to-ground in new rotation; // may split FS's then join FS's

    enable disconnected undergrounds to connect;

    perform queued checks for disconnected undergrounds;
}
buildUndergroundPipe() {
    disable disconnected undergrounds from connecting;

    build pipe-to-ground; // may split FS's then join FS's

    enable disconnected undergrounds to connect;

    perform queued checks for disconnected undergrounds;
}

Every time when FS is split there is need to traverse FS. If you would hit a disconnected underground pair, add it to queue so that there is no performance penalty (second pass over FS's) when doing checks after reenabling ability of blocked undergrounds to connect.

[Dominik]

You are quite right that it is similar, user perspective. Also the rotation code looks kinda similar to what you wrote - i will show how it actually looks (won't copy it as it might not be cool with rules) so that you can reflect
Difference is that all the rotation is done in one place, while construction process has more steps in different places, through code used by other stuff. So doing this thing is by far not that simple. I already tried it yesterday in some form and did not work, will see today what I can do. But I think that I will not be doable reasonably and I will block the action instead. Which will also not be simple but it is doable. What I don't like is that the check is not quite cheap computationally which is annoying for such an edge case.

so, setDirection():

Code: Select all

if has blocked connection, return fail // this is the fix from last week
store whether it has any blocked connection in its fluid system
disable fixing blocks (the scope lock)
full destroy (=including reconfiguring fluid systems)
rotate
light setup
check fluid mixing
if failed, rotate back
light destroy
full setup
end of scope lock
if succeeded and had blocked connection in system, review the system and try to fix

[boskid]

Right, depends on who the Driver is (entity when building or fluidbox(?) when rotate).

So then, another solution: add defered action queue to entity engine. Every time when removing or creating fluidboxes if you would hit blocked underground, add defered action to check that particular connection for possible fix, but all this defered checks would be performed only after all other actions on entity. Something like mentioned "scope lock" but without need to Lock or Unlock: it would be by default locked and "unlocked" for moment (that is, only in defered actions queue it would be possible to fix blocked undergrounds).

This way it would decouple blocked undergrounds fixing from other actions that could happen during building and so it would look like all remove-create actions (in rotate) or create [this issue: internally does disconnect and connect] are performed atomicaly.

-- edit:
in the end, issue is that blocked undergroud fixing is happening in places where it breaks some assumptions:
if you have FB[acid] - FB[acid] - FB[acid], after removing middle fluid box, i would assume there are only 3 cases possible:
- FB[acid] - gap - FB[not assigned]
- FB[not assigned] - gap - FB[acid]
- FB[acid] - gap - FB[acid]
where because of fixing in between, it is possible there will be something like this:
- FB[acid] - gap - FB[crude oil]
only because fixing happend in between

defered fixing should solve this issue and some from previous week

[boskid]

More analysis of issue:

First observation is that fixing unblocking underground is quite easy:

Code: Select all

fixUnderground(fb1, fb2) {
    if(fb1.fluid == "not assigned" || fb2.fluid == "not assigned" || fb1.fluid == fb2.fluid) {
        mergeFluidSystems(fb1, fb2);
        mark undergrounds fb1, fb2 as not blocked
    }
}

This action is however hard to revert: you loose information about which underground was blocked.

----

On the other end you have this case (as above):
- FB[acid] - FB[acid] - FB[acid]

where by removing middle fluidbox i would assume you can only go to this 3 states:
- FB[acid] - gap - FB[not assigned]
- FB[not assigned] - gap - FB[acid]
- FB[acid] - gap - FB[acid]

all of them have one thing in common: they are easy to revert. Just build middle fluidbox to get into state:
- FB[acid] - FB[acid] - FB[acid]

----

Now lets consider building underground as transaction. We want it to be atomic, so all actions are performed or none are performed.

If building underground from this issue is done like this (simplification for sake of reading):

Code: Select all

buildUnderground() {
    // here is stable state A, before transaction

    // building pipe-to-ground
    disconnect overlapping existing underground connection;
    fix blocked undergrounds // THIS action is hard to revert
    put fluid box
    connect it to nearby pipe-to-ground

    if(fluid mixing detected) {
        // fluid mixing detected.
        // we need to revert all above actions to get back into state A
        // but how to revert fixed undergrounds???

        // we need state A, but we get state A with fluid mixing because fixed undergrounds are not reverted

    } else {
        // No mixing, we are done: stable state B
    }
}

This fixing can be postponed (deferred) so it is performed when all actions in transaction are done.

----
For now there are clearly 2 actions that cannot be reverted:
- fixing blocked underground, because you loose information about which undergrounds were fixed
- removing pipe-to-ground which was blocked, because you loose details about which connectors were disconnected (in case of multiple undergrounds in all directions)

So preventing rotation of blocked underground is reasonable - cannot be easily reverted in case of fluid mixing. But upgrading steam engine into steam turbine should be allowed quite easily: when no underground fixing is performed, action can be reverted at any point of transaction.

Case from this bug report also should be easy to fix: when building pipe-to-ground, disconnecting existing underground would not trigger fixing blocked undergrounds, connecting new pipe-to-ground to existing pipe to ground on left would be performed as normal (it was not marked as itself being disconnected) and after confirmation of no fluid mixing with this new pipe (we know state B is reached or reachable, no more need to revert) we may perform fixing blocked undergrouns (no need to revert in this state)

[Dominik]

You are right that it is doable in that way - just build it with the lock and afterwards run the block fix. But I would have to add that into a quite generic construction code and this rare case is just not worth the clutter. So I have done the prevention instead as in the other cases - it won't be buildable and it will say that the blocked connection should be fixed first. Now the blocked connections will be always highlighted and impossible to miss so making the player repair it is very reasonable.

[Dominik]

Oh and btw that wrong block highlight at the start of this thread is also already fixed.

[boskid]

So I have done the prevention instead as in the other cases - it won't be buildable and it will say that the blocked connection should be fixed first.

Well, it is your decision. I will accept this, but we will see if players will be complaining. When enough complain arise then this "defered fix block" solution will be way to go, but for now prevention should be enough.

Now the blocked connections will be always highlighted and impossible to miss so making the player repair it is very reasonable.

Finally! Thanks!

[Dominik]

These blocked pipes are very rare in real games, it's really not a problem to ask people to fix it before doing stuff.

[boskid]

These blocked pipes are very rare in real games, it's really not a problem to ask people to fix it before doing stuff.

I hope it will not be used as a PvP multiplayer strategy to artifitialy create blocked undergrounds far from enemy and then go with long line of pipes/undergrounds to enemy base to prevent any interaction with boilers. Rip steam engines if not possible to fix or find that there is sneaking underground pipe that prevents fixing destroyed boilers or something. Or even blocked undergrounds in enemy team base (they wont see it is blocked as they are in force different from force undergrounds belongs to)

[Dominik]

Interesting line of thinking. I never even played pvp. But in this case, you can just cut off the pipe. And the highlight should always be there, it is force independent.