mapblock_mesh.cpp: Fix code style and simplify a bit code #4558

HybridDog · 2016-09-28T18:24:35Z

No description provided.

Foghrye4 · 2016-10-01T05:41:13Z

src/mapblock_mesh.cpp

 }

 /*
 	Gets nth node tile (0 <= n <= 5).
 */
-TileSpec getNodeTileN(MapNode mn, v3s16 p, u8 tileindex, MeshMakeData *data)
+inline TileSpec getNodeTileN(MapNode mn, v3s16 p, u8 tileindex, MeshMakeData *data)


I'm not a really experienced C++ programmer and so i confused a little bit. I noticed, that gcc C++ compiler at Win platform raise an error undefined reference to getNodeTileN.
So, what happened, when you make inline non-void function?

@HybridDog I've got an info as in this article: https://en.wikipedia.org/wiki/Inline_function
that inline keyword is an instruction for a compiler to replace a function call with a code, which is written in a function body. Therefore I don't quite understand, what compiler would do when function return something and this "something" assigned to object.

I don't think getNodeTileN() can be inlined (not in the manner suggested here anyway). It's used at least 7 times in content_mapblock.cpp and there is a function prototype for it in mapblock_mesh.h.

The real mystery, if there is any, is why it compiles ok on Linux ;)

sfan5 · 2016-10-02T14:15:01Z

To correctly inline functions you need to name them static inline, however it seems to me that inlining these functions isn't that useful. (gcc would do it by itself if it is useful)

Zeno- · 2016-10-07T03:30:54Z

src/mapblock_mesh.cpp

@@ -1389,9 +1329,8 @@ void MapBlockMesh::updateCameraOffset(v3s16 camera_offset)
 {
 	if (camera_offset != m_camera_offset) {
 		translateMesh(m_mesh, intToFloat(m_camera_offset-camera_offset, BS));
-		if (m_enable_vbo) {
+		if (m_enable_vbo)


I don't really think these braces needed to be removed because they were used in a consistent manner. But don't put them back, I'm just making a comment :)

Zeno- · 2016-10-07T03:33:49Z

👍

SmallJoker · 2016-10-07T07:29:57Z

src/mapblock_mesh.cpp

@@ -258,9 +253,8 @@ static u16 getSmoothLightCombined(v3s16 p, MeshMakeData *data)
 			light_day += decode_light(n.getLightNoChecks(LIGHTBANK_DAY, &f));
 			light_night += decode_light(n.getLightNoChecks(LIGHTBANK_NIGHT, &f));
 			light_count++;
-		} else {
+		} else


if (condition) { do_this(); do_that(); } else { otherwise(); }

https://www.kernel.org/doc/Documentation/CodingStyle

SmallJoker · 2016-10-07T07:33:16Z

src/mapblock_mesh.cpp

-	default:
-		break;
+	switch (tile.rotation) {
+		case 0:


This was fine before:

switch (action) { case KOBJ_ADD: return "add"; case KOBJ_REMOVE: return "remove";

(also from the Linux kernel code style guidelines)

SmallJoker · 2016-10-07T07:39:00Z

src/mapblock_mesh.cpp

 			material.setTexture(2, p.tile.flags_texture);
-		} else {
+		} else


Zeno- · 2016-10-07T08:32:11Z

I have no idea if I'm looking at the correct commits LOL. I click View Changes and... not found. Weird, it's always worked before. shrug

HybridDog · 2016-10-07T09:30:01Z

Zeno-, l fixed things while rebasing and then force-pushed.
l do not know how to close a review and if l wrote "fixed" (or sth similar), l would further bloat the page.

Zeno- · 2016-10-07T09:50:31Z

@HybridDog Ahh... I was probably looking at it during the time you were doing that. It explains the issue anyway :)

rubenwardy · 2017-02-13T01:19:36Z

src/mapblock_mesh.cpp

-	if     (scale.X < 0.999 || scale.X > 1.001) abs_scale = scale.X;
-	else if(scale.Y < 0.999 || scale.Y > 1.001) abs_scale = scale.Y;
-	else if(scale.Z < 0.999 || scale.Z > 1.001) abs_scale = scale.Z;
+	if     (scale.X < 0.999 || scale.X > 1.001)


the former is an acceptable exception to the code style IMO (if you add a space after else if)

HybridDog · 2017-08-28T09:45:15Z

please feedback

nerzhul · 2017-08-28T11:52:06Z

wow you changed the whole codestyle, it's huge and difficult to review

nerzhul · 2017-08-28T11:52:29Z

src/mapblock_mesh.cpp

-	}
-	else if(dir == v3s16(0,0,-1))
-	{
+	} else if(dir == v3s16(0,0,-1)) {


missing spaces in all if

nerzhul · 2017-08-28T11:52:51Z

src/mapblock_mesh.cpp

-	if     (scale.X < 0.999 || scale.X > 1.001) abs_scale = scale.X;
-	else if(scale.Y < 0.999 || scale.Y > 1.001) abs_scale = scale.Y;
-	else if(scale.Z < 0.999 || scale.Z > 1.001) abs_scale = scale.Z;
+	if      (scale.X < 0.999 || scale.X > 1.001) abs_scale = scale.X;


can you add f to float values please ?

nerzhul · 2017-08-28T11:53:10Z

src/mapblock_mesh.cpp

-					dest);
-		}
-	}
+	for(s16 z = 0; z < MAP_BLOCKSIZE; z++)


missing spaces

nerzhul · 2017-08-28T11:53:23Z

src/mapblock_mesh.cpp

@@ -1048,8 +1018,8 @@ MapBlockMesh::MapBlockMesh(MeshMakeData *data, v3s16 camera_offset):
 		//TimeTaker timer2("MeshCollector building");

 		for (const FastFace &f : fastfaces_new) {
-			const u16 indices[] = {0,1,2,2,3,0};
-			const u16 indices_alternate[] = {0,1,3,2,3,1};
+			const u16 indices[] = {0, 1, 2, 2, 3, 0};


all those should be static

SmallJoker

LGTM

) * mapblock_mesh.cpp: Fix code style and simplify a bit code

Foghrye4 reviewed Oct 1, 2016

View reviewed changes

HybridDog force-pushed the mameshort branch from 451bf70 to 2f2d00c Compare October 1, 2016 09:21

Zeno- added the Action needed label Oct 1, 2016

sfan5 added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label Oct 2, 2016

HybridDog force-pushed the mameshort branch from 2f2d00c to bdf2780 Compare October 6, 2016 08:41

Zeno- removed the Action needed label Oct 7, 2016

Zeno- reviewed Oct 7, 2016

View reviewed changes

Zeno- added the One approval ✅ ◻️ label Oct 7, 2016

SmallJoker reviewed Oct 7, 2016

View reviewed changes

src/mapblock_mesh.cpp Outdated

material.setTexture(2, p.tile.flags_texture);

} else {

} else

Copy link

Member

SmallJoker Oct 7, 2016

Choose a reason for hiding this comment

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

Same here

HybridDog force-pushed the mameshort branch from bdf2780 to 6d82ccb Compare October 7, 2016 08:14

sfan5 force-pushed the master branch from c90b63a to 09f1a0c Compare December 21, 2016 13:13

HybridDog force-pushed the mameshort branch from 6d82ccb to ced9b6b Compare February 11, 2017 12:12

rubenwardy reviewed Feb 13, 2017

View reviewed changes

paramat added the Rebase needed The PR needs to be rebased by its author. label Mar 26, 2017

HybridDog force-pushed the mameshort branch from 6e59bfe to 2adeab2 Compare May 7, 2017 11:59

HybridDog force-pushed the mameshort branch from 2adeab2 to 955581e Compare August 24, 2017 10:06

nerzhul removed the Rebase needed The PR needs to be rebased by its author. label Aug 28, 2017

nerzhul requested changes Aug 28, 2017

View reviewed changes

HybridDog added 3 commits August 28, 2017 16:49

mapblock_mesh.cpp: Fix code style and simplify a bit code

c432f77

Do what rubenwardy wrote

9ee8bb9

fixes suggested by nerzhul

601f838

HybridDog force-pushed the mameshort branch from 955581e to 601f838 Compare August 28, 2017 14:58

SmallJoker approved these changes Aug 28, 2017

View reviewed changes

SmallJoker added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Aug 28, 2017

nerzhul merged commit 7e38475 into minetest:master Aug 28, 2017

HybridDog deleted the mameshort branch August 28, 2017 18:33

osjc pushed a commit to osjc/minetest that referenced this pull request Jan 11, 2019

mapblock_mesh.cpp: Fix code style and simplify a bit code (minetest#4558

5f1def4

) * mapblock_mesh.cpp: Fix code style and simplify a bit code

osjc pushed a commit to osjc/minetest that referenced this pull request Jan 23, 2019

mapblock_mesh.cpp: Fix code style and simplify a bit code (minetest#4558

67ecbe5

) * mapblock_mesh.cpp: Fix code style and simplify a bit code

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mapblock_mesh.cpp: Fix code style and simplify a bit code #4558

mapblock_mesh.cpp: Fix code style and simplify a bit code #4558

HybridDog commented Sep 28, 2016

Foghrye4 Oct 1, 2016 •

edited

Foghrye4 Oct 1, 2016 •

edited

Zeno- Oct 1, 2016

sfan5 commented Oct 2, 2016

Zeno- Oct 7, 2016

Zeno- commented Oct 7, 2016

SmallJoker Oct 7, 2016 •

edited

SmallJoker Oct 7, 2016

SmallJoker Oct 7, 2016

Zeno- commented Oct 7, 2016

HybridDog commented Oct 7, 2016

Zeno- commented Oct 7, 2016

rubenwardy Feb 13, 2017

HybridDog Feb 13, 2017

HybridDog commented Aug 28, 2017

nerzhul commented Aug 28, 2017

nerzhul Aug 28, 2017

nerzhul Aug 28, 2017

nerzhul Aug 28, 2017

nerzhul Aug 28, 2017

SmallJoker left a comment

mapblock_mesh.cpp: Fix code style and simplify a bit code #4558

mapblock_mesh.cpp: Fix code style and simplify a bit code #4558

Conversation

HybridDog commented Sep 28, 2016

Foghrye4 Oct 1, 2016 • edited

Choose a reason for hiding this comment

Foghrye4 Oct 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfan5 commented Oct 2, 2016

Choose a reason for hiding this comment

Zeno- commented Oct 7, 2016

SmallJoker Oct 7, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zeno- commented Oct 7, 2016

HybridDog commented Oct 7, 2016

Zeno- commented Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HybridDog commented Aug 28, 2017

nerzhul commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmallJoker left a comment

Choose a reason for hiding this comment

Foghrye4 Oct 1, 2016 •

edited

Foghrye4 Oct 1, 2016 •

edited

SmallJoker Oct 7, 2016 •

edited