added music theory

gallant commented

2022-10-01 14:16:43 -05:00

Contributor

idk if this may cause shenanigans

gallant added 1 commit 2022-10-01 14:16:44 -05:00

8658fabc8e added music theory

karx commented

2022-10-01 16:50:43 -05:00

Owner

Great PR! However, there's a couple style changes that I'd like to get out of the way.

play should return the JoinHandle returned by thread::spawn
Please move play out of impl Game. In my opinion, Game should only contain things which directly affect the game loop. play does not.
Please document the play method. Good docs make happy users (and other developers).

Thanks for this PR, if you just fix these things, I'll merge it.

Great PR! However, there's a couple style changes that I'd like to get out of the way. 1. `play` should return the `JoinHandle` returned by `thread::spawn` 2. Please move `play` out of `impl Game`. In my opinion, Game should only contain things which directly affect the game loop. `play` does not. 3. Please document the `play` method. Good docs make happy users (and other developers). Thanks for this PR, if you just fix these things, I'll merge it.

karx commented

2022-10-01 16:53:15 -05:00

Owner

Also, please put this behind a feature flag; we don't want people to pull in dependencies for audio if they're not going to use it.

Thanks!

Also, please put this behind a feature flag; we don't want people to pull in dependencies for audio if they're not going to use it. Thanks!

gallant added 1 commit 2022-10-02 11:12:13 -05:00

b8ddcdb423 moved function and added docs

karx commented

2022-10-02 15:21:05 -05:00

Owner

Thanks for the commit! As before, please put the play function behind a feature flag.

I don't think you need to bind the result of thread::spawn into its own variable here. You should just be able to remove the semicolon on line 793 and take advantage of Rust's implicit return syntax. This way, you can bypass the need for a variable entirely.

Also, if you could, please have play be generic over AsRef<Path> for the path to the audio file? This will allow more flexibility for users, keep allocations to a minimum, and allow users to specify a path containing invalid unicode characters.

If you want, I can merge this into a development branch and make these changes myself; just let me know how you want to proceed with this PR.

Thanks for the commit! As before, please put the `play` function behind a feature flag. I don't think you need to bind the result of `thread::spawn` into its own variable here. You should just be able to remove the semicolon on line 793 and take advantage of Rust's implicit return syntax. This way, you can bypass the need for a variable entirely. Also, if you could, please have `play` be generic over `AsRef<Path>` for the path to the audio file? This will allow more flexibility for users, keep allocations to a minimum, and allow users to specify a path containing invalid unicode characters. If you want, I can merge this into a development branch and make these changes myself; just let me know how you want to proceed with this PR.

gallant commented

2022-10-02 15:53:31 -05:00

Author

Contributor

i'm not exactly sure how to make those changes so it would be really helpful if you did that haha

gallant added 2 commits 2022-10-02 23:50:59 -05:00

22b71a3ffe Made pathing AsRef but with more bounds due to thread shenanigans, locked behind audio feature

d372aedfa2 Made pathing AsRef but with more bounds due to thread shenanigans, locked behind audio feature

gallant commented

2022-10-02 23:51:42 -05:00

Author

Contributor