Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop liquids from flowing over ignore. #2977

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
8 participants
@red-001
Copy link
Contributor

commented Jul 31, 2015

No description provided.

@PilzAdam

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2015

Keeping the nodes that are on top of unloaded blocks constantly in the queue is really bad.

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2015

PilzAdam then how do you update them when the block is loaded?

@PilzAdam

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2015

@red-001 dunno, I guess thats the reason why it wasn't done before ;-)

Maybe add liquids on top of loaded blocks to the update queue? But then it would be quite inconsistent, since other liquids are not updated when loading a block...

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2015

@PilzAdam I checked the memory and CPU usage with and without my modification and I couldn't see any difference. You could always add an option to disable this.

@PilzAdam

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2015

@red-001 well, the CPU usage doesn't increase because the loopcount is limited by liquid_loop_max. The problem is that other liquid nodes will take longer until they are updated.

Also what was your test scenario? Imagine a skyblock game where there are a lot of water and lava streams flowing down on unloaded blocks.

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2015

I tested it in skyblock with 9 water and 9 lava sources.
The update speed seemed to decrees only slightly in with my modification and the cpu usage seemed to be the a slightly higher without it.

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2015

I don't think ignored flows need to be re-added to the queue at all. Any neighboring node that is set would cause that node to get re-added as well, which is coincidentally great for us since that node would never be able to flow until there is a change from content_ignore to something else, anyway.

So, when content_ignore is detected, just skip to the next item.

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2015

@kwolekr I tried that. It didn't work. Nodes aren't add to queue on block load.

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2015

Hrmm. That's true. Maybe you should try a different approach.

Here's what I had in mind: When content_ignore is encountered in a neighbor, add that node position (or maybe block position, or both) to a queue. We'll call it the transform-liquid-in-pending-block-load queue. Next cycle, check if any blocks in that queue have been loaded. If so, add the node position(s) associated with that block to the main transforming liquid queue and remove that block position from the pending load queue.

I realize it is much more complex than what you have, but continuously saturating the liquid transform queue is a pretty bad since it would prevent some nodes from ever getting transformed and it'll make half the liquids on the map "stuck" under some intensive situations, such as caves in Mapgen V5 with lava.

There is a reason why this hasn't already been fixed.

@est31 est31 force-pushed the minetest:master branch to a890c66 Aug 2, 2015

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2015

Wouldn't it be better to just update blocks on load?

@PilzAdam

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2015

@red-001 that may add too much load when people are walking around. Maybe an ABM would be better, although that would require a lot of tuning.

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 23, 2015

Anyone inspired to work on this, using the method suggested by kwolekr?

@@ -1731,7 +1732,11 @@ void Map::transformLiquids(std::map<v3s16, MapBlock*> & modified_blocks)
if (nb.t == NEIGHBOR_LOWER) {
flowing_down = true;
}
} else {
}
else if (nb.n.getContent() == CONTENT_IGNORE){

This comment has been minimized.

Copy link
@nerzhul

nerzhul Oct 24, 2015

Member

space

@red-001 red-001 force-pushed the red-001:fix_water_bugs branch to 82dc0a3 Dec 30, 2015

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2015

IMO. This issue should have high priority as it is often found on actual MP servers and causes different kinds of water problems that is pain to resolve by non-admins.

@gregorycu

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2015

I think red-001's idea about updating on load is good. I understand the potential for a performance problem, however, I've made the lighting code much faster so we have some wiggle room. We should attempt this solution and observe the performance cost.

I agree with kwolekr that saturating the transform queue is horrible. With regards to the transform-liquid-in-pending-block-load queue, that would have to be persisted, if we decided to go down the path.

If we update on load, that may be good enough, and least likely to break.

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2016

Hmm you're right, if the pending queue isn't saved to disk then we could have the situation where liquid flowed into ignore, stopped, added that block to the queue, then the server restarts and leaves off with an empty queue so that block is never updated for liquids. Darn. I had an otherwise solid plan :(

I agree that updating the whole block (or at least the nodes along the edges...) would be the easiest thing to do but that's at the very least 96 nodes added to the queue per load. Too much for my comfort.

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2016

Then again, the liquid transform queue itself has no persistence, so what is it exactly that makes it okay for that situation?

@gregorycu

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2016

Maybe it suffers the same fate? Could be a bug that nobody has reported?

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2016

It's possible. In any case, I'm adding a scan along the edges for liquids in activateBlock(). It'll add any it finds to the transform queue.

This should be a really good balance between performance and implementation simplicity.

@gregorycu

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2016

I think so too.

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2016

No good, I tried that strategy and it keeps the number of items in the liquid transforming queue at around ~12000 when map is loading near ground level. I'm not really comfortable with such an impact for a small edge case like this.

@gregorycu

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2016

I don't understand the maths, to be honest. How can it keep things in the
queue?
On 16/01/2016 4:01 pm, "kwolekr" notifications@github.com wrote:

No good, I tried that strategy and it keeps the number of items in the
liquid transforming queue at around ~12000 when map is loading near ground
level. I'm not really comfortable with such an impact for a small edge case
like this.


Reply to this email directly or view it on GitHub
#2977 (comment).

@gregorycu

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2016

Sorry. It adds new things to the queue constantly to keep it at that level.
I get it now.
On 16/01/2016 4:03 pm, "Gregory Currie" gregory.currie@gmail.com wrote:

I don't understand the maths, to be honest. How can it keep things in the
queue?
On 16/01/2016 4:01 pm, "kwolekr" notifications@github.com wrote:

No good, I tried that strategy and it keeps the number of items in the
liquid transforming queue at around ~12000 when map is loading near ground
level. I'm not really comfortable with such an impact for a small edge case
like this.


Reply to this email directly or view it on GitHub
#2977 (comment).

@paramat

This comment has been minimized.

Copy link
Member

commented Jan 16, 2016

Thanks for the atttempt. I'm thinking perhaps an ABM that occasionally updates nodes (including falling nodes to make the hangng sand created by mapgen collapse, something i was considering anyway). However the way liquid spreads out on ignore should be fixed first because if the whole area of spread water starts flowing down it will look even more ugly and multiply the problem.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2016

Is it possible to activate when player is near in some vicinity (like check for hanging water in 100 blocks or whatever).

Or when liquid stops flowing over ignore, you mark it and store info in block metadata to activate it when approached.

One of the canonical examples of those horrible water bugs: https://www.youtube.com/watch?v=j63mDfJ5rww (starting at 0:30 sec), it shows liquid over ignore bug and other horrible things that go along with it.

Newly merged valleys mapgen caves (mapgen in itself is awesome) are also heavily impacted by liquid over ignore bug.

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2016

@paramat spreading over ignore is easy to fix. This PR already does that, just remove this line and you have the water just stop when it reaches ignore. An ABM could be used to restart it again but that would be feel a bit like a hack that barely works. Water is not done very well in general (e.g place a source block high in the air and then quit and you get water stuck in the air and not flowing) and needs a lot of work to make it work semi-realistically.

@paramat

This comment has been minimized.

Copy link
Member

commented Jan 17, 2016

See #3581

@paramat

This comment has been minimized.

Copy link
Member

commented Jan 23, 2016

kwolekr > No good, I tried that strategy and it keeps the number of items in the liquid transforming queue at around ~12000 when map is loading near ground level.

How about adding to the liquid queue only the flowing liquids in the lowest node layer of a mapblock, on block load? This will cover most cases of stuck liquids. not adding sources avoids huge amounts of sea being added.
I think we need an engine solution, FIxer and i have been experimenting with an ABM but it doesn't work too well and is heavy.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2016

I have a world copy with lots of broken flowing liquids hanging in the air, when I start it freshly near that liquid... closest ones somehow reactivate itself, but if I start far from them - they hang. I'm not sure if this reproducible, or maybe I did something here. Also, if my memory is not wrong 99.9999% of liquid hang is flowing kind (many even without any sources at all).

If some one wants to experience some real bugged liquids, here is the world (go south from spawn to reach increasingly big amounts of them): Download: Just Test.zip

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2016

Maybe do what @paramat say and add an option like "fix_liquids" that will update all the liquids on block load allowing badly bugged maps to be fixed without mods?

@paramat

This comment has been minimized.

Copy link
Member

commented Jan 23, 2016

Actually there is already an API to add a node to the liquid update queue, but it didn't seems to work for me, maybe it wan't acting on the lowest node in a column:

minetest.transforming_liquid_add(pos)

Might be worth experimenting with, it's lighter and doesn't spam the terminal.

when I start it freshly near that liquid... closest ones somehow reactivate itself, but if I start far from them - they hang

Maybe because ABMs have an active range of 32 nodes?

@paramat paramat closed this Feb 18, 2016

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

Closing as my PR was merged. Discussion of water reflow methods is useful.

@MillersMan

This comment has been minimized.

Copy link
Contributor

commented May 1, 2016

I've created a request (#4061) for a water reflow implementation that will solve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.