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

Added /playsound command (missing minVolume) #282

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

Conversation

Croos3r
Copy link

@Croos3r Croos3r commented Nov 15, 2024

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.

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

@Croos3r Croos3r marked this pull request as ready for review November 15, 2024 06:23
@Croos3r Croos3r changed the title Feat/playsound command /playsound command (missing minVolume), Debug traits several data types, refactor/move of sound related data between crates Nov 15, 2024
@Croos3r Croos3r marked this pull request as draft November 15, 2024 06:29
@Croos3r Croos3r marked this pull request as ready for review November 15, 2024 06:29
@Snowiiii
Copy link
Owner

  1. You mean you moved the Sounds from pumpkin-core to pumpkin-macros not the other way around
  2. "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.
  3. 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 Croos3r changed the title /playsound command (missing minVolume), Debug traits several data types, refactor/move of sound related data between crates Added /playsound command (missing minVolume), Added Debug traits several data types, refactor/move of sound related data between crates Nov 15, 2024
@Croos3r
Copy link
Author

Croos3r commented Nov 15, 2024

  1. 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

  1. "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.

  1. 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
Copy link
Owner

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

@Croos3r
Copy link
Author

Croos3r commented Nov 17, 2024

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.

Took 4 hours 15 minutes


Took 5 minutes

Took 14 minutes
The default value for the existing World::play_sound calls is 1.0 for volume and pitch.

Took 7 minutes
…to pumpkin-core and added a parser for the several sound categories

Took 2 hours 15 minutes

Took 1 minute
Took 6 hours 12 minutes

Took 32 seconds
…ound struct fields public

Took 1 hour 56 minutes
@Croos3r Croos3r changed the title Added /playsound command (missing minVolume), Added Debug traits several data types, refactor/move of sound related data between crates Added /playsound command (missing minVolume) Nov 17, 2024
@Snowiiii
Copy link
Owner

Snowiiii commented Nov 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants