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

api: Add support for playing sound with an "emitter" #316

Merged
merged 7 commits into from Jun 7, 2021

Conversation

kezz
Copy link
Member

@kezz kezz commented Mar 27, 2021

This is a simple PR that adds the concept of a sound "player". The interface for Sound.Player is intentionally empty, as the way the client/server identifies a sound player is with the entity ID, an implementation detail that some platforms may not wish to expose.

The general idea is that a native implementation will implement Sound.Player on their entity class, allowing it to be passed directly as an argument into the playSound method. For platforms, they could provide utility methods to convert their entity class to an implementation of the Sound.Player interface which provides an entity object, or some other form of identifier. This can then be used to convert to the platform specific way of sending the entity sound effect packet.

Alternatively, the Sound.Player could provide an integer ID, but this is just exposing what is essentially a magic number, not something as constant as, say, a UUID. I'm also not 100% sold on the Player part, I think it's potentially too confusing with the actual player. Perhaps something like Sound.Maker or Sound.EntitySource?

@kashike kashike linked an issue Mar 30, 2021 that may be closed by this pull request
@kezz kezz changed the title api: Add support for playing sound with a "player" api: Add support for playing sound with an "emitter" Mar 31, 2021
@kezz
Copy link
Member Author

kezz commented Apr 9, 2021

Just a thought: it's worth noting that whilst this does fix #315, it doesn't "fix" the subsequent problem of having a method to play sounds coming from the audience member (unless an implementation decides to use Audience#playSound(Sound) for that).

The issue arises from the fact that if you have a ForwardingAudience, you can't run code like forwardingAudience.playSound(sound, forwardingAudience) or something similar, you'd have to iterate through each member of the audience and play the sound to them directly. This isn't a terrible solution, as in most implementations you'd be able to do code like:

for (Player player : world.getPlayers()) {
    player.playSound(sound, player);
}

However, this does mean that there isn't a cross-platform way to signal you'd like a sound to be played from each member of the audience and it is a potential problem with Adventure that there isn't a way to indicate this. Firstly, I believe that it should be specified exactly what happens with the Audience#playSound(Sound) method to avoid confusion across implementations as to how the sound would be played. Additionally, a method that mimics Audience#playSound(Sound) could be created to allow playing sound using each audience member as the Emitter, perhaps, as was suggested in PaperMC/Paper#5472, an additional boolean parameter to specify if the sound should follow the receiver or not.

@kashike
Copy link
Member

kashike commented Apr 9, 2021

playSoundFromEmitter?

@kezz
Copy link
Member Author

kezz commented Apr 9, 2021

playSoundFromEmitter?

Yeah I suppose that could work - I think it does potentially confuse the idea of Emitter as opposed to the "receiver" of a sound.

@kezz
Copy link
Member Author

kezz commented Jun 4, 2021

Rebased and added further clarification to the Javadocs for the added method/interface.

@zml2008 zml2008 added this to In progress in Adventure via automation Jun 5, 2021
@zml2008 zml2008 moved this from In progress to Reviewer approved in Adventure Jun 5, 2021
@kezz
Copy link
Member Author

kezz commented Jun 6, 2021

Following on from zml's comment in #330 (comment), I have updated this PR to add Emitter#self() which, when passed into Audience#playSound(Sound, Emitter) should cause the sound to be played from the recipient of the sound.

@kezz kezz requested a review from zml2008 June 6, 2021 21:13
Adventure automation moved this from Reviewer approved to Review in progress Jun 6, 2021
@zml2008 zml2008 added this to the 4.8.0 milestone Jun 6, 2021
Adventure automation moved this from Review in progress to Reviewer approved Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add a way to send an Audience a sound playing from an entity
4 participants