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

Fix invalid block entities created during world gen #10375

Merged
merged 3 commits into from
Apr 6, 2024

Conversation

QuickGlare
Copy link
Contributor

This patch fixes #10022 and sachingorkar102/Lootin-plugin#24

When using WorldGenRegion#setBlock on a block position where a BlockEntity exists it doesn't remove the old one, this creates a broken state in the world and makes the clients crash due to an internal server error.

To fix these issue, I'm removing the BlockEntity when WorldGenRegion#setBlock is set (to fix new worlds) and I've changed the LevelChunk#tick method to remove the invalid BlockEntity (to fix the existing worlds)

@QuickGlare QuickGlare requested a review from a team as a code owner March 30, 2024 20:26
Copy link
Member

@electronicboy electronicboy left a comment

Choose a reason for hiding this comment

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

The paper start comment should be moved a line up and that diff can be somewhat reduced, and the end command should copy the start description; merger can deal with this, however, if desired

@QuickGlare
Copy link
Contributor Author

@electronicboy I found another issue related to this (even if it's not the same) so I'm going to make another PR to fix it

Basically, if a Sculk Sensor is in a broken state (Block and Block Entity aren't right) the vibration system makes the player crash, this PR doesn't fix it

net.minecraft.ReportedException: Ticking player at net.minecraft.server.level.ServerPlayer.doTick(ServerPlayer.java:856) ~[main/:?] at net.minecraft.server.network.ServerGamePacketListenerImpl.tick(ServerGamePacketListenerImpl.java:338) ~[main/:?] at net.minecraft.network.Connection.tick(Connection.java:592) ~[main/:?] at net.minecraft.server.network.ServerConnectionListener.tick(ServerConnectionListener.java:234) ~[main/:?] at net.minecraft.server.MinecraftServer.tickChildren(MinecraftServer.java:1746) ~[main/:?] at net.minecraft.server.dedicated.DedicatedServer.tickChildren(DedicatedServer.java:447) ~[main/:?] at net.minecraft.server.MinecraftServer.tickServer(MinecraftServer.java:1525) ~[main/:?] at net.minecraft.server.MinecraftServer.runServer(MinecraftServer.java:1226) ~[main/:?] at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:319) ~[main/:?] at java.lang.Thread.run(Thread.java:833) ~[?:?] Caused by: java.lang.IllegalArgumentException: Cannot get property EnumProperty{name=sculk_sensor_phase, clazz=class net.minecraft.world.level.block.state.properties.SculkSensorPhase, values=[inactive, active, cooldown]} as it does not exist in Block{minecraft:sculk_catalyst} at net.minecraft.world.level.block.state.StateHolder.getValue(StateHolder.java:95) ~[main/:?] at net.minecraft.world.level.block.SculkSensorBlock.getPhase(SculkSensorBlock.java:216) ~[main/:?] at net.minecraft.world.level.block.SculkSensorBlock.canActivate(SculkSensorBlock.java:220) ~[main/:?] at net.minecraft.world.level.block.entity.SculkSensorBlockEntity$VibrationUser.canReceiveVibration(SculkSensorBlockEntity.java:129) ~[main/:?] at net.minecraft.world.level.gameevent.vibrations.VibrationSystem$Listener.handleGameEvent(VibrationSystem.java:292) ~[main/:?] at net.minecraft.world.level.gameevent.GameEventDispatcher.lambda$post$0(GameEventDispatcher.java:50) ~[main/:?] at net.minecraft.world.level.gameevent.EuclideanGameEventListenerRegistry.visitInRangeListeners(EuclideanGameEventListenerRegistry.java:74) ~[filteredMinecraft.jar:?] at net.minecraft.world.level.gameevent.GameEventDispatcher.post(GameEventDispatcher.java:62) ~[main/:?] at net.minecraft.server.level.ServerLevel.gameEvent(ServerLevel.java:1833) ~[main/:?] at net.minecraft.world.entity.Entity.vibrationAndSoundEffectsFromBlock(Entity.java:1332) ~[main/:?] at net.minecraft.world.entity.Entity.move(Entity.java:1226) ~[main/:?] at net.minecraft.world.entity.LivingEntity.handleRelativeFrictionAndCalculateMovement(LivingEntity.java:2958) ~[main/:?] at net.minecraft.world.entity.LivingEntity.travel(LivingEntity.java:2892) ~[main/:?] at net.minecraft.world.entity.player.Player.travel(Player.java:1682) ~[main/:?] at net.minecraft.server.level.ServerPlayer.travel(ServerPlayer.java:1726) ~[main/:?] at net.minecraft.world.entity.LivingEntity.aiStep(LivingEntity.java:3474) ~[main/:?] at net.minecraft.world.entity.player.Player.aiStep(Player.java:570) ~[main/:?] at net.minecraft.world.entity.LivingEntity.tick(LivingEntity.java:3066) ~[main/:?] at net.minecraft.world.entity.player.Player.tick(Player.java:274) ~[main/:?] at net.minecraft.server.level.ServerPlayer.doTick(ServerPlayer.java:770) ~[main/:?] ... 9 more

@QuickGlare
Copy link
Contributor Author

The paper start comment should be moved a line up and that diff can be somewhat reduced, and the end command should copy the start description; merger can deal with this, however, if desired

I can move the line up, how do I reduce the diff?

@electronicboy
Copy link
Member

cleaning up the diff is moreso just accepting that the thing is going to look at bit messy, i.e. reverting the indentation changes;

I was hoping that the tick logic would get to the block entity before it blew up, will probably have to just make the server check that the blockstate is valid for any block entity sets during loading in order to remove them earlier

@QuickGlare
Copy link
Contributor Author

cleaning up the diff is moreso just accepting that the thing is going to look at bit messy, i.e. reverting the indentation changes;

I was hoping that the tick logic would get to the block entity before it blew up, will probably have to just make the server check that the blockstate is valid for any block entity sets during loading in order to remove them earlier

Yes, that's a better way to do it, do you want me to add that in this PR or another one?

@electronicboy
Copy link
Member

generally that sorta thing would be done as a separate PR

@lynxplay lynxplay merged commit 06361fa into PaperMC:master Apr 6, 2024
3 checks passed
lynxplay added a commit to lynxplay/paper that referenced this pull request May 26, 2024
The previous PR PaperMC#10375 removes block entities on setBlock calls to the
WorldGen region to prevent the existence of a block entity on a block
that does not support it as such.

The logic however assumed that such calls would always follow the order
of:
  1. set the base blockstate
  2. create the block entity
  3. finish world gen

This is however not the case for all of world gen, specifically not for
structure templates being placed under water, as these first set the
block state, then configure its block entity (include the chests
lootable) and finally waterlogs the chest by calling another setBlock.

To prevent this logic from deleting the original block entity, this
commit now only removes the block entity found if the blockstate to be
set differs from the previous one, matching LevelChunk#setBlockState.
lynxplay added a commit to lynxplay/paper that referenced this pull request May 27, 2024
The previous PR PaperMC#10375 removes block entities on setBlock calls to the
WorldGen region to prevent the existence of a block entity on a block
that does not support it as such.

The logic however assumed that such calls would always follow the order
of:
  1. set the base blockstate
  2. create the block entity
  3. finish world gen

This is however not the case for all of world gen, specifically not for
structure templates being placed under water, as these first set the
block state, then configure its block entity (include the chests
lootable) and finally waterlogs the chest by calling another setBlock.

To prevent this logic from deleting the original block entity, this
commit now only removes the block entity found if the blockstate to be
set differs from the previous one, matching LevelChunk#setBlockState.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to handle packet
4 participants