Properly handle generators with null spawns #536
Conversation
| spawnX += random.nextInt(256) - 128; | ||
| spawnZ += random.nextInt(256) - 128; | ||
| } | ||
| setSpawnLocation(spawnX, getHighestBlockYAt(spawnX, spawnZ), spawnZ); |
There was a problem hiding this comment.
Wouldn't it be getHighestBlockYAt(spawnX, spawnZ) + 1? Or is the 1 offset handled when actually spawning the player?
There was a problem hiding this comment.
Spawn point is a solid block
There was a problem hiding this comment.
Are you sure? If you use /setworldspawn, it would set the spawn at the location of the executor which is 1 block above the surface.
There was a problem hiding this comment.
My mistake, but I know that getHighestBlockYAt produces the correct Y value for the spawn point, whatever that value is supposed to be.
There was a problem hiding this comment.
Alright, I guess the Y position doesn't really matter anyways because it is adjusted to the correct height when a player spawns (to avoid suffocation).
| int spawnX = random.nextInt(256) - 128, spawnZ = random.nextInt(256) - 128; | ||
| getChunkAt(spawnX >> 4, spawnZ >> 4).load(true); // I'm not sure there's a sane way around this | ||
|
|
||
| for (int tries = 0; tries < 1000 && !generator.canSpawn(this, spawnX, spawnZ); ++tries) { |
There was a problem hiding this comment.
1000 seems rather high, especially if all attempts going to fail because the spawn chunk is in the ocean or something.
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm sure I'm missing something, but why not just use that method instead of duping the code? Also, that doesn't change that 1000 is a lot :P
There was a problem hiding this comment.
Because that method isn't static.
I didn't change the value because that's outside of the scope of the PR. I could expand the scope to change the loop to a more appropriate value, and add a static helper method for default random spawn finding code.
There was a problem hiding this comment.
generator.getFixedSpawnLocation(this, random)? It wouldn't need to be static
There was a problem hiding this comment.
https://github.com/GlowstoneMC/Glowstone/pull/536/files#diff-cea1d25e5518f2495b11e2d20021b0a0R767
The whole point of this PR is to add a case for when generator.getFixedSpawnLocation(this, random) returns null (as mentioned in the PR post "This fixes a null pointer exception when a generator returns a null spawn").
There was a problem hiding this comment.
Oh, I see. I should have read the PR description better then, my bad :)
This fixes a null pointer exception when a generator returns a null spawn. This is valid according to the Bukkit spec and means that the server should find a random spawn point.