Implement a proper solution to stop the music when a jukebox is destroyed #5800
Conversation
dktapps
added
Category: API
This is already being called in `Block::onDestroy()`
| /** | ||
| * Called when this block is destroyed either when a player breaks it or is hit by an explosion. | ||
| */ | ||
| public function onDestroy() : void{ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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($tile !== null){ | ||
| $tile->onBlockDestroyed(); | ||
| } | ||
| $target->onDestroy(); |
There was a problem hiding this comment.
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.
|
Has anyone considered using I haven't thought much about the pros and cons of this approach, but I think it should be considered. |
|
Linking #4652 as I think these are two different solutions for essentially the same problem |
|
This PR has been marked as "Waiting on Author", but we haven't seen any activity in 7 days. If there is no further activity, it will be closed in 28 days. Note for maintainers: Adding an assignee to the PR will prevent it from being marked as stale. |
![pmmp-admin-bot[bot]](https://avatars.githubusercontent.com/in/1059952?s=60&v=4){kind=small}
| $t->onBlockDestroyed(); | ||
| } | ||
| $world->setBlock($this->position, VanillaBlocks::AIR()); | ||
| $this->position->getWorld()->setBlock($this->position, VanillaBlocks::AIR()); |
There was a problem hiding this comment.
Noting this is probably BC breaking, since the behaviour is now changed for non-core consumers of onBreak()
|
The disc does actually appear to drop on an explosion. It's probably because the yield is less than 100% that you didn't see that. The main deviation from vanilla would appear to be that the disc doesn't drop when the jukebox is broken by a creative player. So something akin to #4652 would probably be better suited to dealing with that problem, or we could have it drop in There is also the issue of containers dropping their contents in |
3 participants
Introduction
The current solution presents some problems as first of all Tiles should not handle the behavior of blocks, instead of using the jukebox API to eject the disc directly boradcasts
RecordStopSound, which causes the loss of the disc by destroying the jukebox which at the same time does not match the Vanilla behavior.This PR addresses the problems mentioned above by adding the
Block::onDestroy()method, making the block not dependent on the Tile to manage this logic, which as a plus eliminates duplicate code inBlock::onBreak()andExplosion::explodeB().