Added /playsound command (missing minVolume)

Croos3r commented 1 week ago

Description

This PR adds a first incomplete implementation of the playsound command (the minVolume is just ignored and not used for the moment). It also adds the Debug trait on an amount of data structures And finally it moves the sound data structure from pumpkin-macros to pumpkin-core

Testing

The command has been tested functionnaly in game with various inputs.

Checklist

Things need to be done before this Pull Request can be merged.

[x] Code is well-formatted and adheres to project style guidelines: cargo fmt
[x] Code does not produce any clippy warnings cargo clippy

Snowiiii commented 1 week ago

You mean you moved the Sounds from pumpkin-core to pumpkin-macros not the other way around
"Debug traits several data types" is not a descrption, You just removed most of the Debug traits, So you could just say that you removed the Debug trait from most Structs.
Please don't try to do everything in one Pull Request. You should use multiple PRs for all changes. And if something like the /playsound command is not ready you should mark the PR as a draft

Croos3r commented 1 week ago

You mean you moved the Sounds from pumpkin-core to pumpkin-macros not the other way around

No, I moved the Sound data stuctures from pumpkin-macros to pumpkin-core, but I saw that it might be good to add it to pumpkin-registry. Tell me and I will do accordingly

"Debug traits several data types" is not a descrption, You just removed most of the Debug traits, So you could just say that you removed the Debug trait from most Structs.

Yes, my bad, I forgot a word, I added them to several structs to make everyone able to debug theses structs.

Please don't try to do everything in one Pull Request. You should use multiple PRs for all changes. And if something like the /playsound command is not ready you should mark the PR as a draft

The playsound command only miss the minVolume argument. As I'm not sure if I will be able to work more on it for the next days I submitted it. The command can already be used but it just miss this part.

I will try to split the command, the debug trait add and the move of sound structures to different PR

Snowiiii commented 6 days ago

So, Why you added a bunch of Debug's ?

Croos3r commented 5 days ago

I had to add them to debug and understand how the command dispatcher was working. It was a bit painful to add them everywhere in the code so I figured I will left them here to make it easier for the next person that wants to debug

For the Sound data structs move to core should I open another PR ? It's a really small change and is directly related to the command.

Snowiiii commented 4 days ago

I had to add them to debug and understand how the command dispatcher was working. It was a bit painful to add them everywhere in the code so I figured I will left them here to make it easier for the next person that wants to debug

For the Sound data structs move to core should I open another PR ? It's a really small change and is directly related to the command.

This is not a good way to debug things. You basically just added them EVERYWHERE. at this point im wondering if you even read the code or just working out of your terminal and can only see what the Debug traits prints. Pumpkin's Code is not bad. I would say it is in fact pretty good. There are not many deep rabbit holes and you can relatively easily see how everything works.

For the Sound data structs move to core should I open another PR ? It's a really small change and is directly related to the command.

I don't know. I did not look at your Code because i don't want to search actual code trough all the 43 Changed files. If you need it for the /playsound command it only makes sense to have them in the same PR

Croos3r commented 2 days ago

This is not a good way to debug things. You basically just added them EVERYWHERE. at this point im wondering if you even read the code or just working out of your terminal and can only see what the Debug traits prints. Pumpkin's Code is not bad. I would say it is in fact pretty good. There are not many deep rabbit holes and you can relatively easily see how everything works.

I am pretty new to Rust. I didn't understood some of the code that was running and just wanted to have a look on specific data at a specific time to help myself understand what I was doing wrong. I figured using the dbg! macro was a good way to do that. But the structure I wanted to print in dbg! was not implementing debug and so the types in that structure, etc. I derive(Debug)'d where I had to (to make the code with dbg! compile), to have an overview of the data structure. I asked to some friend that are way better in Rust than I am and they told me that implementing Debug where I could was always a good choice, I also asked on the Discord what to do and the answer was to implement it where it wasn't.

I will delete this PR and start over with whatever you want me to do with it:

Adding or not the Debug trait where I already put it.
Moving or not the sound related data structures from pumpkin-protocol/src/lib.rs to pumpkin-core/src/sound.rs

I am sorry if the way I did things is bad or rude to you, I do not often contribute to OSS and am not aware of the way things works. Genuinely just wanted to help on a cool project

Tell me how to do things and I'll do accordingly

Snowiiii / Pumpkin