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

Support for Soundboard and VC effects #9349

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

Puncher1
Copy link
Contributor

@Puncher1 Puncher1 commented Apr 9, 2023

Summary

This PR adds following:

  • Support for the VOICE_CHANNEL_EFFECT_SEND event
  • Support for soundboard sounds
    • create_, edit_, delete_
    • Fetch default soundboard sounds
    • Request guild soundboard sounds over the gateway
    • soundboard_sounds property and get_ method (Guild and Client)
    • GUILD_SOUNDBOARD_CREATE, GUILD_SOUNDBOARD_UPDATE, GUILD_SOUNDBOARD_DELETE events
  • Related Audit Log events

Current Problems

Current "problems" on Discord's side, which is stated in the related soundboard PR

  • There is no GET route for individual sounds or a guild's list of sounds. Guild sound lists can currently only be retrieved via the Gateway. GET routes should be added *
  • PATCHing a sound currently clears the sound's volume and emoji if those aren't included in the request. The PR documents this current behavior, let me know once it is fixed **
  • Looks like id and sound_id are always the same, but id is optional ***

* As I tested myself, you can also get the soundboard sounds in the GUILD_CREATE event (e.g. on startup).
** Not handled or documented as for now
*** sound_id is always used, id is not implemented

Others

Things which aren't documented in the related Discord PR:

  • override_path is not included in any payload anymore -> both default and guild sounds uses an ID now, so e.g. the "bad umm tss" sound's ID is 7 and so the cdn url is https://cdn.discordapp.com/soundboard-sounds/7

Related PRs: discord/discord-api-docs#6025 (voice channel effects), discord/discord-api-docs#6260 (soundboard)


Checklist

Testing pending

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

Finished: VC effect + VC sound effect
Started: Soundboard support
@Puncher1 Puncher1 marked this pull request as draft April 9, 2023 14:27
@Rapptz Rapptz added the pending PR implements an in-progress or explicitly unreleased Discord feature label Apr 18, 2023
@SvenLie
Copy link

SvenLie commented Jun 20, 2023

When can we expect this to be undrafted and merged? @Puncher1

@Puncher1
Copy link
Contributor Author

@SvenLie The GET method is still not supported which, at least for me, doesn't make sense to merge it yet.

@Puncher1
Copy link
Contributor Author

Update

  • For the current status, please refer to the PR description
  • The 3.x checks are failing. Looks like there's an error in the installation, @Rapptz
  • Testing is pending, after that this PR is ready for review

@Rapptz
Copy link
Owner

Rapptz commented Oct 29, 2023

3.x is failing due to aiohttp not building on 3.12 and 3.x is latest Python, so CI is blocked on dependencies and not much I can do there.

@Puncher1 Puncher1 marked this pull request as ready for review November 2, 2023 19:14
@Puncher1
Copy link
Contributor Author

Puncher1 commented Nov 2, 2023

This has been tested and is ready for review now.

@PaulMarisOUMary
Copy link

I believe the issues with 3.x have been resolved now. 🎉

Copy link
Owner

@Rapptz Rapptz left a comment

Choose a reason for hiding this comment

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

I've only done a cursory pass at this. It has merge conflicts currently as well, though this feature is pretty complicated and definitely needs documentation on Discord's end for me to merge it.

There are a few things I'm kind of iffy about, some more important than others:

  1. The name SoundboardSound is pretty silly, I'm unsure if going Soudboard is any better but it doesn't read as oddly.
  2. The machinery to handle opcode 31 in the gateway seems very complicated for what it is. The chunking mechanism is the way it is because it's expected that multiple events flow in and it needs to be synced and waited. I have no reason to believe soundboard sounds operate the same way but I also have no idea if they do. I tried asking to get clarification on this though I haven't gotten a response yet. I'll update you when I do.

discord/channel.py Show resolved Hide resolved
discord/channel.py Outdated Show resolved Hide resolved
discord/channel.py Outdated Show resolved Hide resolved
discord/utils.py Outdated Show resolved Hide resolved
discord/channel.py Outdated Show resolved Hide resolved
discord/soundboard.py Outdated Show resolved Hide resolved
discord/state.py Outdated Show resolved Hide resolved
discord/asset.py Outdated Show resolved Hide resolved
@Puncher1
Copy link
Contributor Author

Alright I'm done with the changes. Because of the name, how about just Sound? And should it be also applied for properties and attributes (e.g. Client.soundboard_sounds -> Client.sounds)?

@Puncher1 Puncher1 requested a review from Rapptz January 27, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending PR implements an in-progress or explicitly unreleased Discord feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants