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

Cleanup in content_mapblock #5746

Merged
merged 1 commit into from
May 20, 2017
Merged

Cleanup in content_mapblock #5746

merged 1 commit into from
May 20, 2017

Conversation

numberZero
Copy link
Contributor

NDT_LIQUID is being drawn by MapBlockMesh since a long time ago...

NDT_LIQUID is being drawn by MapBlockMesh since a long time ago...
@nerzhul
Copy link
Member

nerzhul commented May 12, 2017

testing it

@nerzhul nerzhul added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label May 12, 2017
@@ -1291,8 +1270,7 @@ void MapblockMeshGenerator::drawNode()
else
light = getInteriorLight(n, 1, nodedef);
switch (f->drawtype) {
case NDT_LIQUID: drawLiquidNode(false); break;
case NDT_FLOWINGLIQUID: drawLiquidNode(true); break;
case NDT_FLOWINGLIQUID: drawLiquidNode(); break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work if NDT_LIQUID is not handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NDT_LIQUID is being drawn by MapBlockMesh since a long time ago...

as it has solidness=1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah now I see why this can be removed. That's quite new land to me, thus thanks for pointing this out :)

@nerzhul
Copy link
Member

nerzhul commented May 13, 2017

Here is the result of this PR

https://youtu.be/YIk5ZuIBc_s

If it's this function (please confirm) i approve it

@@ -559,7 +543,7 @@ void MapblockMeshGenerator::drawLiquidSides(bool flowing)
}
}

void MapblockMeshGenerator::drawLiquidTop(bool flowing)
void MapblockMeshGenerator::drawLiquidTop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please rename these functions to drawFlowingLiquid{*}, for easier understanding?

@numberZero
Copy link
Contributor Author

If it's this function

pardon?

Actually I was surprised by the fact liquids are not drawn in content_mapblock, despite it having all the code. But:
nodedef.cpp, line 685:

        case NDT_LIQUID:
                assert(liquid_type == LIQUID_SOURCE);
                if (tsettings.opaque_water)
                        alpha = 255;
                solidness = 1;

content_mapblock.cpp, line 1323→1301:

	if (f->solidness != 0)
		continue;

That is, NDT_LIQUID was skipped here. So the code I removed was not used at all.

@nerzhul
Copy link
Member

nerzhul commented May 14, 2017

i would mean, if it's dead code for flowing liquids i'm fine with this

@numberZero
Copy link
Contributor Author

@nerzhul Yes, I removed dead code only.

@SmallJoker
Copy link
Member

Applied this patch and played a bit with the buckets. Works as expected. 👍

@nerzhul nerzhul merged commit 7779bac into minetest:master May 20, 2017
@numberZero numberZero deleted the content-mapblock-cleanup branch June 5, 2017 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants