Conversation
dktapps
left a comment
There was a problem hiding this comment.
This was mostly discussed on Discord, but I'll leave some comments here too.
I haven't reviewed this fully yet so this is just a few nits.
| $this->collisionBoxes = null; | ||
|
|
||
| if(($block = $this->getDisplacedBlock())->getTypeId() !== BlockTypeIds::AIR){ | ||
| $block->position($world, $x, $y, $z); |
There was a problem hiding this comment.
This seems a bit sketch. In the default case, this will allocate a new air block, position it, and then immediately throw it away. That doesn't seem good for performance.
There was a problem hiding this comment.
Should probably be fine now, as getDisplacedBlock() now returns null in most cases.
…to waterlogging
|
vanilla: vanilla.mp4this rp: pr.mp4 |
|
Once again Bedrock makes no sense... |
src/item/LiquidBucket.php
Outdated
| $targetBlock = $coveredBlock; | ||
| $resultBlock = $this->liquid instanceof Water ? | ||
| (clone $targetBlock)->setWaterCover((clone $this->liquid)->setStill(false)) : | ||
| clone $this->liquid; |
There was a problem hiding this comment.
Isn't this going to cause partial-loggable blocks to get replaced by lava? This doesn't feel right, if we have any partial-loggable blocks that have canBeReplaced() = false in the future, this is going to cause unexpected behaviour
There was a problem hiding this comment.
e.g. if a plugin made signs partial-loggable, they'd get replaced here by lava
There was a problem hiding this comment.
I've changed the logic, but it's still remains incorrect. Vanilla for some reason allows some non-source waterloggable blocks to be replaced by lava here. Yeah, Bedrock again makes no sense, and I don't know if we want to entirely fix it (we would probably have to add something like Block->canBeReplacedBy(Block $block) : bool).
src/world/World.php
Outdated
|
|
||
| if($block instanceof CoveredByWater && $displacedBlock !== Block::EMPTY_STATE_ID){ | ||
| $displacedBlock = $this->blockStateRegistry->fromStateId($displacedBlock); | ||
| $block->setWaterCover($displacedBlock instanceof Water ? $displacedBlock : null); |
There was a problem hiding this comment.
Hmm. Maybe we need some more general way to deal with this, will need to think about this more
…to waterlogging
…overedByFlowing to type tag
Fixed, thank you! |
| UpdateBlockPacket::FLAG_NETWORK, | ||
| UpdateBlockPacket::DATA_LAYER_NORMAL | ||
| ); | ||
| if($fullBlock->getTypeId() === BlockTypeIds::AIR || $fullBlock instanceof Waterloggable){ |
There was a problem hiding this comment.
This is needed to sync the second layer if a waterlogged block was destroyed by plugins, explosions, etc.
There was a problem hiding this comment.
Hmm, I feel like this isn't reliable. The block could've been directly replaced by something else
There was a problem hiding this comment.
I can’t come up with any other reliable ways to save the bandwidth, as we always need to keep the client in sync with the current blocks anyway.
|
I just haven’t made trapdoor to be waterloggable yet. I added waterlogging functionality just to few basic blocks to not add a bunch of noise to the diff and make reviewing waterlogging logic easier, this is why it’s marked as incomplete. When the final API is fully discussed, I’ll make all the remaining blocks waterloggable. |
Introduction
This PR implements support for waterlogging. For now I've marked this PR as "incomplete" only because I added waterlogging support for basic blocks (stairs, slabs, doors, levers, fences) to make sure everything matches vanilla and to not add a lot of noise to the diff. When the API is fully discussed and we make sure everything works fine, I'll make other remaining blocks to be waterloggable.
Related issues & PRs
#3722
Changes
API changes
Added
CoveredByWaterinterface, as well asCoveredByWaterTrait. Blocks which implement this interface are automatically considered waterloggable.As Bedrock allows flowing water to waterlog some blocks too, I've decided to keep Water block inside of Waterloggable one, so the API is similar to the plant blocks in decorated pot.
To change block's waterlogging state there is a
CoveredByWater->setWaterCover(?Water $waterCover) : self.To get the water cover there is a
CoveredByWater->getWaterCover() : ?Water.Example usages:
$block->setWaterCover(VanillaBlocks::WATER());$block->setWaterCover(VanillaBlocks::WATER()->setDecay(5));$block->setWaterCover(null);To check that the block is waterlogged you can do
$block->getWaterCover() !== null.Internal API changes
On the Internal level there is a new
displaced blockabbreviation, which refers to a block, placed on the second layer.The following internal public methods have been added, which should NOT be used by plugins:
World->delayDisplacedBlockUpdate(Vector3 $pos, int $delay) : voidSimilar toscheduleDelayedBlockUpdate(), but for displaced blocks. Unfortunately, I had to add this to avoid collisions with blocks which use a regular schedule delayed updates apart from being waterloggable (e.x campfire, brewing stand for cooking). The name is probably quite odd, but I want to make sure no one accidentally uses this internal method instead of the regular one by IDE autocomplete.Block->onDisplacedScheduledUpdate() : voidSimilar toonScheduledUpdate(), but called for displaced blocks.Block->getDisplacedBlock() : BlockReturns AIR by default, overriden by waterloggable blocks to return their water state. Called in the core to retrieve a displaced block to send an update block packet, write that block to the second layer, run a separate schedule delayed update, update its position.Block->setDisplacedBlock(Block $block) : voidcalled duringgetBlockAt()to retrieve a block from the second layer.Backwards compatibility
Plugins which interact with the second layer via internal methods (like this one) will stop working because blocks, which are not expected to be in the second layer, will be overwritten by air.
Follow-up
Implement snowlogging, it can be done very quickly after these changes.
Tests
A small video can be found here: https://www.youtube.com/watch?v=_HQG7-bTJXg
From what can I see, it seems like it's fully matches vanilla behaviour, but I'm not 100% sure.
Note: in the end you might see that a lava can't collide with a non-waterlogged lever. I have fixed that bug after video recording.