Nether Portal Jumping & Creation Code #2162

lkolbly · 2015-05-29T21:23:55Z

Per #1856, this code implements a portal scanner which finds and creates nether portals when entities move between the overworld and the nether.

It also changes dae9e579#diff-97bcf8a96a22aec81982926aa1462363R298 because StringToInteger returns true if the conversion was successful. I think.

worktycho · 2015-05-29T21:31:58Z

src/BlockID.cpp

@@ -295,7 +295,7 @@ eDimension StringToDimension(const AString & a_DimensionString)
 {
 	// First try decoding as a number
 	int res;
-	if (!StringToInteger(a_DimensionString, res))


Thanks, I got a few of these wrong.

I'm going to manually apply this patch to master, because its an important fix.

madmaxoft · 2015-05-29T21:38:39Z

Please try to use the same coding style. We use a space after all commas, and a space around pointer and reference declarations (cWorld * a_World etc.) There's too many of these to report each one. Unfortunately the basic stylechecking script cannot detect these reliably, so you'll need to do it by hand.

lkolbly · 2015-05-29T21:40:37Z

Okay, I'll go back through as I move it to a new file.

NiLSPACE · 2015-05-29T21:53:14Z

src/Entities/Entity.cpp

+{
+	if (m_FoundPortal)
+	{
+		m_PortalLoc = m_PortalLoc;


I doubt this is needed.

Good catch, thanks.

lkolbly · 2015-05-30T01:40:52Z

Okay, I think I got it compliant with the coding convention in bad47a8, except maybe local variable names, I don't know what the convention is for those. I didn't see any commas without spaces, though, so I'm probably looking at the wrong commas?

madmaxoft · 2015-05-30T05:19:53Z

src/Entities/Entity.cpp

+		m_ReadyToChangeWorlds = false;
+		MoveToWorld(m_NewWorld, false, m_NewWorldPosition);
+		return true;
+	}


This block deserves a comment above it explaining what it is doing.

I think a simple /* If a move to another world was scheduled via ScheduleMoveToWorld(), move the entity now. */ would have been enough. This is too wordy and still doesn't explain much. Also note that comments inside functions shouldn't use the doxy-comment format (/**), only comments above declarations should use it.

Also the m_ReadyToChangeWorlds is a somewhat misleading name, m_IsWorldChangeScheduled might have been better.

lkolbly · 2015-05-30T13:52:29Z

What's the preferred way to convert between Vector types, e.g. from Vector3i to a Vector3d?

madmaxoft · 2015-05-31T07:23:43Z

I don't think there's any preference. There's a simple copy-constructor provided within the vector template, and for converting to a Vector3i there's also the Floor() function defined on the class (Ceil() is missing right now, but could be added with minimal effort).

lkolbly · 2015-06-02T15:05:38Z

Okay. I think I've fixed the convention errors.

madmaxoft · 2015-06-03T08:16:22Z

src/NetherPortalScanner.cpp

+	m_Dir = 0;
+	m_Position = a_DestPosition;
+	m_PortalLoc = a_DestPosition.Floor();
+	m_MaxY = a_MaxY;


These should go in the initialization section of the constructor

lkolbly · 2015-06-06T15:46:20Z

Alright, I just refactored m_Dir to use a Direction type which is either X or Y (I considered NORTH_SOUTH and EAST_WEST, but that didn't seem to map to the code as well).

madmaxoft · 2015-06-08T21:39:45Z

Is there any chance you could get this rebased onto current master and squashed into a single commit?

lkolbly · 2015-06-08T23:35:30Z

Sure thing. Done.

madmaxoft · 2015-06-09T13:41:09Z

I think this is merge-worthy. Opinions?

NiLSPACE · 2015-06-09T15:13:06Z

src/Entities/Entity.h

@@ -530,6 +535,11 @@ class cEntity
 	eEntityType m_EntityType;

 	cWorld * m_World;
+
+	/// State variables for ScheduleMoveToWorld.
+	bool m_IsWorldChangeScheduled = false;


Shouldn't this be initialized in the construcor? That is where we usualy initialize member variables right?

And also /** */ for comment style.

Yes, it should be initialized in the constructor. For the comment, there was a previous block of several comments there that were triple slashes, so I changed the whole block in the commit I just now pushed.

Yeah, sorry for the comment style, we couldn't decide on the format beforehands, so one half of the code uses /// and the other half uses /** */. We've since then decided to use the latter, but didn't fix the former yet. Good call about replacing them all.

NiLSPACE · 2015-06-09T15:16:59Z

Apart from the question I have about the m_IsWorldChangeScheduled initialization it looks really good.

ghost · 2015-06-09T15:22:40Z

Please add yourself to the contributors file.

tigerw · 2015-06-09T15:31:31Z

src/NetherPortalScanner.h

+	virtual bool OnAllChunksAvailable(void) override;
+	virtual void OnDisabled(void) override;
+
+	enum Direction


Maybe enum class Direction?

sphinxc0re · 2015-06-10T00:35:37Z

Looks like it's ready for merge

Nether Portal Jumping & Creation Code

NiLSPACE · 2015-06-13T10:14:34Z

After almost a month I finally updated my MCServer and tried this, and I noticed that when teleporting to an portal I get teleported 1 block next to it. That probably isn't intended right? You're supposed to teleport in another portal.

EDIT:
I also noticed that if I create a portal on the top of the nether, go through it, and then go back in the portal I get teleported to the top of the nether. Vanilla doesn't allow this to happen.

sphinxc0re · 2015-06-13T10:26:23Z

Also, you get teleported instantly but I think this is another issue

NiLSPACE · 2015-06-13T10:26:48Z

That's only in creative gamemode right? Because in vanilla you also get teleported instantly in creative mode.

NiLSPACE · 2015-06-13T10:32:54Z

src/NetherPortalScanner.cpp

+	m_World->GetChunkBlockTypes(a_ChunkX, a_ChunkZ, blocks);
+
+	// Iterate through all of the blocks in the chunk
+	for (unsigned int i = 0; i < cChunkDef::NumBlocks; i++)


Looking at this again, shouldn't this be i += 2 as suggested by @madmaxoft here? It would speed up the search for a nether portal by quite allot since it wouldn't have to check every block in the chunk.

Because of the way the chunks are stored (XZY?), doing i+2 here would result in a pattern that skips every other row. Note that it's iterating from 0 to NumBlocks, not 0 to 16 each axis - this results in a speedup in not having to call CoordinateToIndex.

It would have been possible to have two loops, the inner one iterating over the 256 blocks in a single Y slice, and the outer one iterating over Y slices, skipping 2 slices out of each 3:

for (unsigned y = 0; y < cChunkDef::Height; y += 3) { unsigned BaseY = cChunkDef::Width * cChunkDef::Width * y; for (unsigned i = 0; i < 256; i++) { // Check block at index [BaseY + i] } }

ghost · 2015-06-13T10:42:00Z

I tested this, I can not get back to the world from the nether trough the portal. Used the FAQ for creating a nether world.

NiLSPACE · 2015-06-13T10:48:04Z

That's probably because you haven't configured the world linking in the world.ini. The FAQ only describes how you can generate such a world.

sphinxc0re · 2015-06-13T10:49:14Z

Yes, this is a problem. You have to stand right in the portal to teleport. in vanilla you only need partial contact with the portal.

Also, you are getting damage for travelling to the nether

tigerw · 2015-06-13T11:55:44Z

Yes, this is a problem. You have to stand right in the portal to teleport. in vanilla you only need partial contact with the portal.

This is to do with lack of bounding box detection in MCS at the present.

lkolbly · 2015-06-13T12:32:30Z

@NiLSPACE

... and I noticed that when teleporting to an portal I get teleported 1 block next to it.

That was intended, but can be changed easily.

NiLSPACE · 2015-06-13T12:37:03Z

I think it should be changed, because I got teleported in the portal frame once. It could also be possible to be teleported inside a wall if the portal is next to it.

lkolbly · 2015-06-13T14:01:32Z

I'm not sure you can since it places the portal in a 3x4x5 empty space, so there's always air on either side of the portal. But I can change it anyway.

NiLSPACE · 2015-06-13T14:14:24Z

But if I build a portal against a wall then there isn't an empty space around it. ^^

lkolbly · 2015-06-13T14:18:18Z

Oh, that's true. I hadn't thought about manually created portals. I will change that & the Y limit and make a PR after #2237 is closed. Having multiple forks is so much effort.

lkolbly referenced this pull request May 29, 2015

Made -Weverything an error.

dae9e57

worktycho reviewed May 29, 2015
View reviewed changes

NiLSPACE reviewed May 29, 2015
View reviewed changes

madmaxoft reviewed May 30, 2015
View reviewed changes

madmaxoft reviewed Jun 3, 2015
View reviewed changes

lkolbly force-pushed the master branch from dfd2f41 to 0361910 Compare June 8, 2015 23:34

NiLSPACE reviewed Jun 9, 2015
View reviewed changes

tigerw reviewed Jun 9, 2015
View reviewed changes

Implemented nether portal scanning code.

73b6a44

lkolbly force-pushed the master branch from 0361910 to 73b6a44 Compare June 9, 2015 23:24

madmaxoft added a commit that referenced this pull request Jun 10, 2015

Merge pull request #2162 from lkolbly/master

d436a71

Nether Portal Jumping & Creation Code

madmaxoft merged commit d436a71 into cuberite:master Jun 10, 2015

NiLSPACE reviewed Jun 13, 2015
View reviewed changes

Nether Portal Jumping & Creation Code #2162

Nether Portal Jumping & Creation Code #2162

Conversation

lkolbly commented May 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madmaxoft commented May 29, 2015

lkolbly commented May 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkolbly commented May 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkolbly commented May 30, 2015

madmaxoft commented May 31, 2015

lkolbly commented Jun 2, 2015

Choose a reason for hiding this comment

lkolbly commented Jun 6, 2015

madmaxoft commented Jun 8, 2015

lkolbly commented Jun 8, 2015

madmaxoft commented Jun 9, 2015

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

NiLSPACE commented Jun 9, 2015

ghost commented Jun 9, 2015

Choose a reason for hiding this comment

sphinxc0re commented Jun 10, 2015

NiLSPACE commented Jun 13, 2015

sphinxc0re commented Jun 13, 2015

NiLSPACE commented Jun 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 13, 2015

NiLSPACE commented Jun 13, 2015

sphinxc0re commented Jun 13, 2015

tigerw commented Jun 13, 2015

lkolbly commented Jun 13, 2015

NiLSPACE commented Jun 13, 2015

lkolbly commented Jun 13, 2015

NiLSPACE commented Jun 13, 2015

lkolbly commented Jun 13, 2015