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.

Sign up for GitHub

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

Implement a proper solution to stop the music when a jukebox is destroyed #5800

Open
wants to merge 6 commits into
base: minor-next
Choose a base branch
from
Open

Implement a proper solution to stop the music when a jukebox is destroyed #5800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d43fb73
Always eject disc when jukebox is destroyed
IvanCraft623 Jun 3, 2023
c87bc6b
Merge remote-tracking branch 'upstream/minor-next' into jukebox
IvanCraft623 Jun 4, 2023
d581c58
Revert dylan's solution
IvanCraft623 Jun 4, 2023
a588225
Oops
IvanCraft623 Jun 4, 2023
b4e17fa
Move `Block::onDestroy()` call to `World::destroyBlockInternal()`
IvanCraft623 Jun 7, 2023
a573060
Remove unnecessary call to `Tile::onBlockDestroyed()`
IvanCraft623 Jun 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/block/Block.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,14 +427,19 @@ public function getBreakInfo() : BlockBreakInfo{
* @param Item[] &$returnedItems Items to be added to the target's inventory (or dropped, if full)
*/
public function onBreak(Item $item, ?Player $player = null, array &$returnedItems = []) : bool{
$world = $this->position->getWorld();
if(($t = $world->getTile($this->position)) !== null){
$t->onBlockDestroyed();
}
$world->setBlock($this->position, VanillaBlocks::AIR());
$this->position->getWorld()->setBlock($this->position, VanillaBlocks::AIR());
return true;
}

/**
* Called when this block is destroyed either when a player breaks it or is hit by an explosion.
*/
public function onDestroy() : void{
Copy link
Member

Choose a reason for hiding this comment

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

Like @dktapps I don't think that destroy terminology is appropriate in this case. Perhaps, as with the placement logic, an onPostBreak would be preferable? It would be less ambiguous and not so funny in the sense that the block would already have been destroyed.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that's desirable. If the block was already destroyed, contents of stuff like tiles wouldn't be accessible.

Personally I would lean towards an onExplode() method or something similar. We already have the likes of onIncinerate() for fire. The problem is that this would require multiple handlers to be implemented for dealing with tile drops.

if(($t = $this->position->getWorld()->getTile($this->position)) !== null){
$t->onBlockDestroyed(); //needed to create drops for inventories
}
}

/**
* Called when this block or a block immediately adjacent to it changes state.
*/
Expand Down
6 changes: 3 additions & 3 deletions src/block/Jukebox.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ public function stopSound() : void{
$this->getPosition()->getWorld()->addSound($this->getPosition(), new RecordStopSound());
}

public function onBreak(Item $item, ?Player $player = null, array &$returnedItems = []) : bool{
$this->stopSound();
return parent::onBreak($item, $player, $returnedItems);
public function onDestroy() : void{
$this->ejectRecord();
parent::onDestroy();
}

public function getDropsForCompatibleTool(Item $item) : array{
Expand Down
5 changes: 0 additions & 5 deletions src/block/tile/Jukebox.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
use pocketmine\item\Record;
use pocketmine\nbt\tag\CompoundTag;
use pocketmine\network\mcpe\convert\TypeConverter;
use pocketmine\world\sound\RecordStopSound;

class Jukebox extends Spawnable{
private const TAG_RECORD = "RecordItem"; //Item CompoundTag
Expand Down Expand Up @@ -63,8 +62,4 @@ protected function addAdditionalSpawnData(CompoundTag $nbt) : void{
$nbt->setTag(self::TAG_RECORD, TypeConverter::getInstance()->getItemTranslator()->toNetworkNbt($this->record));
}
}

protected function onBlockDestroyedHook() : void{
$this->position->getWorld()->addSound($this->position, new RecordStopSound());
}
}
4 changes: 1 addition & 3 deletions src/world/Explosion.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,7 @@ public function explodeB() : bool{
$this->world->dropItem($pos->add(0.5, 0.5, 0.5), $drop);
}
}
if(($t = $this->world->getTileAt($pos->x, $pos->y, $pos->z)) !== null){
$t->onBlockDestroyed(); //needed to create drops for inventories
}
$block->onDestroy();
$this->world->setBlockAt($pos->x, $pos->y, $pos->z, $airBlock);
}
}
Expand Down
6 changes: 1 addition & 5 deletions src/world/World.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2012,11 +2012,7 @@ private function destroyBlockInternal(Block $target, Item $item, ?Player $player
}

$target->onBreak($item, $player, $returnedItems);

$tile = $this->getTile($target->getPosition());
if($tile !== null){
$tile->onBlockDestroyed();
}
$target->onDestroy();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this function only be called if the onBreak returns true? There is a return value, but it's never used, and yet it would make sense in this case. If the broken action has been confirmed, then we can call this function, which is there to perform post-broken actions.

}

/**
Expand Down