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

Add loaded blocks to transforming_liquid-queue similar to updateLiquid #4061

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
6 participants
@MillersMan
Copy link
Contributor

commented May 1, 2016

This adds liquid nodes of blocks loaded from data-base to ServerMap::m_transforming_liquid with the same abstract logic as in MapGen::updateLiquid and has the following fixes:

  • Liquid from above will start flowing into a newly loaded block
  • Unfinished transformations inside the block will resume after loading
  • Liquid in newly loaded blocks will start flowing to next lower block if that was already loaded
  • Only nodes that actually need to be transformed are added to the queue (unlike MapGen::updateLiquid which always adds the whole topmost plane)
@MillersMan

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2016

I've tested with https://github.com/minetest/minetest/files/101456/Just.Test.zip
It looks good so far. However in very few cases some nodes aren't transformed. There is nothing special about those nodes, as they aren't at block borders and after a server restart those are transformed like all others.

@Zeno-

This comment has been minimized.

Copy link
Contributor

commented May 1, 2016

Looks extremely promising

@paramat

This comment has been minimized.

Copy link
Member

commented May 1, 2016

Related #2977
See coments form here onwards #2977 (comment) the concern is too many additions to the liquid queue. Maybe we should consider just updating the nodes in the lowest layer of nodes in a mapblock, as these are the ones most likely to get stuck?

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented May 1, 2016

Could you profile to see how many nodes get added to the liquid transform queue on average per block load?

@MillersMan

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2016

I did tests with valley mapgen and the results so far are:

  • Worst case: Shallow ocean ~100 transformations per block (would expect ~500 for the blocks themselves, but average is over all blocks including air, ground and 100% water which all have no transformations)
  • Surface and ground of deep oceans: below 100 per block (don't have exact values at it is spoiled with result from worst case scenario, would expect ~250 but as before the outcome is average)
  • Valleys with rivers: Below 10 transformations per block (would expect 2 per column if column is higher than one node, otherwise it would be one. So here it is the same, blocks without liquids drop the rate)
  • Deserts or other areas without liquids: No transformations at all
@MillersMan

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2016

It would even be possible to reduce it close to zero if surrounding nodes are checked, too. But this might bring a small burden during the scan phase (will do a separate performance tests for this) would expect about 5% extra scan cost at loading time to only queue nodes for transformation that will change the environment,

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented May 2, 2016

I've tried it with #4065 and have not seen any issues with reflow yet, it activates nicely, even huge amounts, rarely you need to revisit place (fly away and come back) to fix rare pieces.
I've looked both "just test" glitches and mgvalleys. Good so far. max_lag was mostly below 0.4 in singleplayer. When noclipping besides huge lava lakes underground may rise up to 0.8
Update:
By this time I've only notice this curious thing: https://i.imgur.com/z7q3WVb.jpg sometimes huge flows in huge places can interrupt for few blocks if you fly around away from them (liquids keep flowing correctly, so gap will go down until everything is one piece again). Not a big issue though, maybe some excess calculation in such cases? I guess it is because of chunk loading, when middle chunk is not loaded yet it makes below one to go down and upper one to reflow :)
You can test this by creating same world, by using mgvalleys and seed in my picture and placing lots of lava sources on ceiling, make sure to create long and big flows and fly around around them. Or in any other big place with long flows.

@MillersMan

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2016

The issue observed by Fixer with the gaps will be solved when the #4065 changes are also applied to the loading algorithm. The problem is in fact that the upper node wasn't loaded yet so there was a ignored-node above that was interpreted like air. 4065 solves this by not activating blocks below ignored ones at all

@nerzhul

View changes

src/emerge.cpp Outdated
{
MutexAutoLock envlock(m_server->m_env_mutex);

if(block)

This comment has been minimized.

Copy link
@nerzhul

nerzhul Jun 1, 2016

Member

space, and test this before locking to reduce the useless locking (which costs).

@paramat paramat added the WIP label Jun 20, 2016

@paramat paramat added this to the 0.4.15 milestone Oct 8, 2016

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 8, 2016

@MillersMan are you able to finish this for release? (feature freeze early December).
Is there much to do or is it just waiting for reviews?
The gaps are not an issue.

@MillersMan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2016

@paramat There are a few bigger issues that have to be addressed. Loaded blocks are far more difficult than initially created blocks. Those are the issues on my list:

  • ignored nodes above or to the side should prevent flowing away as they might support the stream when loaded (affects normal flow algorithm)
  • ignored nodes below liquid nodes should not result in horizontal spreading (affects normal flow algorithm), i think this was already implemented
  • after loading: air below liquids should result in reflow
  • after loading: liquid above solid or source nodes should spread horizontally to air or to liquid nodes with to low level
  • reflow has to respect the border-nodes of blocks next to the loaded block
  • reflow has to be applied to all loaded blocks, currently some blocks are loaded via different function calls that don't result in a reflow-scan

I think all of them are solvable but my original approach would make this to complex and slow. Maybe i should scan in three separate passes, one for each axis. This might result in a node being added three times to the liquid-update-queue in the very unlikely worst case scenario but it should have an acceptable overall performance.

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

ignored nodes below liquid nodes should not result in horizontal spreading (affects normal flow algorithm), i think this was already implemented

Yes i did that.

If you want a simplified solution the main problem is reflowing liquids in the lowest layer of nodes in a block. This would enable water columns to drop a long distance and be used for travel. I would be happy with just that fixed.

@MillersMan MillersMan force-pushed the MillersMan:reflow_fix branch Nov 27, 2016

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2016

Quoting @MillersMan :

#4061 is updated and in the final state (or at least close to)

Testing time!

src/map.cpp Outdated
flowing_down = true;
if(nb.n.getContent() == CONTENT_IGNORE) {
// If neutral below is ignore prevent water spreading outwards
// otherwise prevent flowing away as neutral node might be a source

This comment has been minimized.

Copy link
@paramat

paramat Nov 27, 2016

Member

If not already please split comment at 80 columns (with tab size 4).

src/map.cpp Outdated
if (nb.t == NEIGHBOR_LOWER &&
nb.n.getContent() == CONTENT_IGNORE)
flowing_down = true;
if(nb.n.getContent() == CONTENT_IGNORE) {

This comment has been minimized.

Copy link
@paramat

paramat Nov 27, 2016

Member

Space after 'if'

This comment has been minimized.

Copy link
@MillersMan

MillersMan Nov 27, 2016

Author Contributor

Ok, I'll fix those

src/map.cpp Outdated
if(nb.n.getContent() == CONTENT_IGNORE) {
// If neutral below is ignore prevent water spreading outwards
// otherwise prevent flowing away as neutral node might be a source
if(nb.t == NEIGHBOR_LOWER)

This comment has been minimized.

Copy link
@paramat

paramat Nov 27, 2016

Member

Space after 'if'

src/map.cpp Outdated
@@ -2894,8 +2904,11 @@ void ServerMap::loadBlock(std::string sectordir, std::string blockfile,
block->deSerialize(is, version, true);

// If it's a new block, insert it to the map
if(created_new)
if(created_new) {

This comment has been minimized.

Copy link
@paramat

paramat Nov 27, 2016

Member

Space after 'if'

src/map.cpp Outdated
@@ -2961,8 +2974,11 @@ void ServerMap::loadBlock(std::string *blob, v3s16 p3d, MapSector *sector, bool
block->deSerialize(is, version, true);

// If it's a new block, insert it to the map
if(created_new)
if(created_new) {

This comment has been minimized.

Copy link
@paramat

paramat Nov 27, 2016

Member

Space after 'if'

src/reflowscan.cpp Outdated
int bz = (MAP_BLOCKSIZE + z) / MAP_BLOCKSIZE;
int idx = (bx + (by * 9) + (bz * 3));
MapBlock *result = m_lookup[idx];
if(!result && (m_lookup_state_bitset & (1 << idx)) == 0) {

This comment has been minimized.

Copy link
@paramat

paramat Nov 27, 2016

Member

Space after 'if'

src/reflowscan.cpp Outdated
{
bool valid_position;
MapBlock *block = lookupBlock(x, y, z);
if(block) {

This comment has been minimized.

Copy link
@paramat

paramat Nov 27, 2016

Member

Space after 'if'

src/reflowscan.cpp Outdated

// Is the column inside a loaded block?
MapBlock *block = lookupBlock(x, 0, z);
if(!block)

This comment has been minimized.

Copy link
@paramat

paramat Nov 27, 2016

Member

Space after 'if'

src/reflowscan.cpp Outdated
for (s16 y = MAP_BLOCKSIZE - 1; y >= 0; y--) {
MapNode node = block->getNodeNoCheck(dx, y, dz, &valid_position);
const ContentFeatures &f = m_ndef->get(node);
bool is_ignored = node.getContent() == CONTENT_IGNORE;

This comment has been minimized.

Copy link
@paramat

paramat Nov 27, 2016

Member

Perhaps 'is_ignore' instead?

This comment has been minimized.

Copy link
@MillersMan

MillersMan Nov 27, 2016

Author Contributor

Maybe, but if it is set I'll skip the rest of the computation and just store it as was_ignored for the next cycle. So it isn't wrong either

src/reflowscan.cpp Outdated
if(below) {
MapNode node = below->getNodeNoCheck(dx, MAP_BLOCKSIZE - 1, dz, &valid_position);
const ContentFeatures &f = m_ndef->get(node);
bool is_ignored = node.getContent() == CONTENT_IGNORE;

This comment has been minimized.

Copy link
@paramat

paramat Nov 27, 2016

Member

'is_ignore'?

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2016

Big 👍 from me (from players perspective testing), I've checked and observed many liquid flows on:

  • just test world - all craziest stuck water stuff went down as it should
  • lavaflow - huge lava flows in huge valleys megacave, observed no gaps, no big reflows as before
  • v7 and mgvalleys world - no problems near oceans, near rivers, good water flows
  • lava flowing in v7 caves - very nice results even for hugely complex long flows
  • good fps, not much mapblock changes
  • good flow, no gaps, very rare activity in massive flows that cancels out.

I'm very satisfied for now.

Prevent waterfalls from falling down or streams from flowing away whe…
…n the source node is in an unloaded block

- Nodes near a CONTENT_IGNORE node will be interpreted as if the ignored node is a liquid node that just supports the current state of the nodes in question

@MillersMan MillersMan force-pushed the MillersMan:reflow_fix branch Nov 27, 2016

src/reflowscan.cpp Outdated
if (is_ignore || was_ignore || is_liquid == was_liquid) {
// Neither topmost node of liquid column nor topmost node below column
} else if (is_liquid) {
// This is the topmost node in the column and might wan't to flow away

This comment has been minimized.

Copy link
@paramat

paramat Nov 28, 2016

Member

wan't -> want

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

Ok i mostly understand this and it looks good, there may be some more technical aspects i don't understand so i'm giving half a +1.

@MillersMan MillersMan force-pushed the MillersMan:reflow_fix branch 2 times, most recently Nov 28, 2016

When loading a block add liquid-nodes that might flow away or spread …
…to neighbours to the transforming_liquid queue

@MillersMan MillersMan force-pushed the MillersMan:reflow_fix branch to 499e436 Nov 28, 2016

@paramat paramat removed the WIP label Nov 29, 2016

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

screenshot_20161129_015555

Tested that stuck water columns under floatlands reflow with this PR.
Since i've tested this, mostly understand the code and since Fixer has tested thoroughly i feel i can give an official 👍 But another dev should check the code.

@Zeno-

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2016

👍 if @MillersMan can clarify that the issues described in the comment on 26 Oct have been addressed

@MillersMan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2016

@Zeno- Everything described in my comment on Oct 25 is fixed. This includes the water-columns mentioned in the paramat's comment on Oct 26.

@paramat

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

Great, added Zeno-'s approval.

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2016

Wow, this looks fantastic. I'm sorry but I haven't had the time to actually do a review on this, since it does actually require some time and effort to carry out.

Would just like to point out a few minor style errors to fix on merge.

{
}

void ReflowScan::scan(MapBlock * block, UniqueQueue<v3s16> *liquid_queue)

This comment has been minimized.

Copy link
@kwolekr

kwolekr Dec 6, 2016

Contributor

Asterisk should be attached to block

class MapBlock;

class ReflowScan
{

This comment has been minimized.

Copy link
@kwolekr

kwolekr Dec 6, 2016

Contributor

This should be on the previous line

This comment has been minimized.

Copy link
@MillersMan

MillersMan Dec 7, 2016

Author Contributor

Really? Typically class, struct and function bodys begin on a separate line while other blocks start on the same line as the declaration. Haven't seen any place in MT source where it is different.

bool isLiquidFlowableTo(int x, int y, int z);
bool isLiquidHorizontallyFlowable(int x, int y, int z);
void scanColumn(int x, int z);
private:

This comment has been minimized.

Copy link
@kwolekr

kwolekr Dec 6, 2016

Contributor

This should be a blank line

@@ -174,7 +175,7 @@ bool Map::isNodeUnderground(v3s16 p)
bool Map::isValidPosition(v3s16 p)
{
v3s16 blockpos = getNodeBlockPos(p);
MapBlock *block = getBlockNoCreate(blockpos);
MapBlock *block = getBlockNoCreateNoEx(blockpos);

This comment has been minimized.

Copy link
@kwolekr

kwolekr Dec 6, 2016

Contributor

Huh, this isn't related to the PR... should it be a separate commit instead, despite its size? Call it a bugfix.

This comment has been minimized.

Copy link
@paramat

paramat Dec 6, 2016

Member

Looks like it is a seperate commit already.

@paramat

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

@MillersMan i'll merge this in 12 hours. If you have no time to fix the 3 code style issues i'll do this myself on merge.

@MillersMan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2016

@paramat Sorry wanted to take a look on the issue earlier but there was another urgent issue. Would be grateful if you could do this on merge.

@paramat

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

No problem.

@paramat

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

@paramat paramat closed this Dec 7, 2016

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2016

Forgot to say. I waited for this for many years, I can't believe it is done and in. Good job, my dreams of reflow come true 🥇

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.