Skip to content

Conversation

Timongcraft
Copy link
Contributor

@Timongcraft Timongcraft commented Sep 3, 2024

With this PR, the Adventure-provided sound API is partially implemented.

It adds the ability to play sounds for a player (from the player itself or other players on the same server) and to stop sounds,
while making the methods fail silently if the sound can't be played/stopped.

What this PR implements:

  • A method to play back (custom) sounds for a specific player (from the player itself or other players) for 1.19.3+
    (- This also enables the respective methods on the registered server, by forwarding it to the players)
  • A method to stop (custom) sounds for players with version 1.19.3+

What this PR doesn't implement:

  • A method to play back (custom) sounds for a specific player at a position or another entity for players of all versions
  • A method to play back (custom) sounds (globally) at a specific location for players of all versions
  • A easily way to access a list of vanilla sounds
    - A method to play back (custom) sounds for a specific player for players with version 1.19.2 and below
    - A method to stop (custom) sounds for players with version 1.19.2 and below

Clarification:
This PR only implements a sound API for version 1.19.3+ because only in this version is the server able to play a (vanilla) sound for a player without requiring a version-dependent id for the sound.

@Timongcraft

This comment was marked as resolved.

@Timongcraft Timongcraft marked this pull request as ready for review September 15, 2024 19:08
@Timongcraft Timongcraft force-pushed the feature/sound-api branch 2 times, most recently from e91c70a to 87b0e57 Compare October 26, 2024 12:45
@nicolube
Copy link

nicolube commented Dec 8, 2024

What is the state of this? I would like to have this feature very much.

@Timongcraft
Copy link
Contributor Author

From my end, it is finished, although I would think it makes more sense to make the methods fail silently again.

@Timongcraft
Copy link
Contributor Author

Timongcraft commented Dec 8, 2024

We could also look into playing sounds from another player, but if so, I think that should be discussed first.

@nicolube
Copy link

@astei Could this be reviewed pls.

@nicolube
Copy link

@4drian3d Since it has been approved, can this also be merged?

@TheXplore
Copy link

What is the state of this? We are currently discussing implementing this ourselves, but we would way prefer having it implemented in upstream.

@Timongcraft
Copy link
Contributor Author

Apart from maybe adding Javadoc to the RegisteredServer, this PR is ready from my end.

@TheXplore
Copy link

@4drian3d Do you think additional Javadoc entries are required? Is there an estimate when this might get merged?

@nicolube
Copy link

nicolube commented Apr 1, 2025

@electronicboy
Sry for the ping, but what is blocking this from being merged?

@Timongcraft
Copy link
Contributor Author

Even though the sound should be ignored for invalid entity ids, I still choose to test for the current server and thus require it on the receiver because afaik they are not that unique and if there's some other entity with that id exists, it could cause unwanted consequences.

@Krymonota
Copy link

Is there any chance that this could be merged soon? I'd love to use it! :)

@Timongcraft
Copy link
Contributor Author

Timongcraft commented Jun 16, 2025

Is there any chance that this could be merged soon? I'd love to use it! :)

I'd wait for 1.21.6 and see how to handle the implementation of Adventure for the new "UI" sound category.

Edit: Seems like it was just added to the end of the enum, in vanilla and adventure, so it shouldn't be a problem.

@Timongcraft Timongcraft force-pushed the feature/sound-api branch 2 times, most recently from efac451 to 6514063 Compare June 27, 2025 16:06
@TheXplore
Copy link

Hi, is there a current estimate when this might get merged? :)

@4drian3d 4drian3d added the type: feature New feature or request label Sep 24, 2025
Copy link
Contributor

@4drian3d 4drian3d left a comment

Choose a reason for hiding this comment

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

lgtm
It works quite well

Copy link
Contributor Author

@Timongcraft Timongcraft left a comment

Choose a reason for hiding this comment

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

I did not implement the Player#playSound(Sound) method as adventure defines it like this:

Plays a sound at the location of the recipient of the sound.

Since we can't fulfill that, would it be fine to break that contract by replacing the javadoc?

https://github.com/KyoriPowered/adventure/blob/-/api/src/main/java/net/kyori/adventure/audience/Audience.java#L629C6-L631

@4drian3d
Copy link
Contributor

I did not implement the Player#playSound(Sound) method as adventure defines it like this:

Plays a sound at the location of the recipient of the sound.

Since we can't fulfill that, would it be fine to break that contract by replacing the javadoc?

https://github.com/KyoriPowered/adventure/blob/-/api/src/main/java/net/kyori/adventure/audience/Audience.java#L629C6-L631

I'll be honest, I tried this pull request for 30 minutes without knowing why the sound wasn't playing, since stopSound was working for me, until I saw that the #playSound(Sound) method wasn't implemented

From a plugin developer's perspective, this method should be implemented, but from a contract compliance perspective, it's okay not to implement it as required by the contract. With this in mind, good documentation at https://docs.papermc.io/velocity/dev/pitfalls/#audience-operations-are-not-fully-supported would suffice, so I am now reverting my change

@Timongcraft
Copy link
Contributor Author

What do you say about stuffing override dialog api methods into this pr as well for documentation purposes?

@4drian3d 4drian3d merged commit c8c27af into PaperMC:dev/3.0.0 Sep 29, 2025
1 check passed
@Timongcraft Timongcraft deleted the feature/sound-api branch September 29, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants