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

Fix AudioStreamPlayback's _stop() not being called when AudioStreamPlayer's stop() is called #97625

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

SourceOfHTML
Copy link
Contributor

@SourceOfHTML SourceOfHTML commented Sep 29, 2024

Small fix that resolves #97466 , although I am not very familiar with that part of the code, and where and how the line would be best implemented, because I have very little idea what the rest of the code in the relevant method is about. (Note: Done with assistance from anvilfolk in the Contributor's Chat)

@SourceOfHTML SourceOfHTML requested a review from a team as a code owner September 29, 2024 16:22
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Hi there! Thanks for your first contribution! We're close, thanks to you!

@SourceOfHTML
Copy link
Contributor Author

what issue does it specifically have with the code style, and how do I get to fixing it?

@adamscott
Copy link
Member

what issue does it specifically have with the code style, and how do I get to fixing it?

I suggest that you install pre-commit and run pre-commit init inside the project before committing. It makes sure that style checks are done before CI.

I suggest that you run git commit --amend afterwards, it will run the pre-commit checks.

@adamscott adamscott changed the title Fixed Playback's _stop() not being called when StreamPlayer's stop() is called Fix AudioStreamPlayback's _stop() not being called when AudioStreamPlayer's stop() is called Sep 30, 2024
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Should be good to merge after the style checks. I tested it and it works well!

@SourceOfHTML
Copy link
Contributor Author

Alright, I reckon the correct version of clang-tools is also necessary? One from https://github.com/llvm/llvm-project/releases/tag/llvmorg-17.0.1 ? I was putting off downloading clang-tools because I don't know which of the files in that list I should download

@adamscott
Copy link
Member

what issue does it specifically have with the code style, and how do I get to fixing it?

I think it's because the in-between blank line has whitespace, just make sure that it's an empty line without tabs.

@adamscott
Copy link
Member

Alright, I reckon the correct version of clang-tools is also necessary? One from https://github.com/llvm/llvm-project/releases/tag/llvmorg-17.0.1 ? I was putting off downloading clang-tools because I don't know which of the files in that list I should download

On which OS are you? You don't have to download the files manually.

@SourceOfHTML
Copy link
Contributor Author

SourceOfHTML commented Sep 30, 2024

Windows, specifically Windows10

@adamscott
Copy link
Member

Windows, specifically Windows10

I think that by installing LLVM with scoop, you'll be able to have clang-format.

@SourceOfHTML
Copy link
Contributor Author

I got LLVM and clang-format, woo! Thank you!

@SourceOfHTML SourceOfHTML requested review from a team as code owners October 2, 2024 06:34
@SourceOfHTML SourceOfHTML requested review from a team as code owners October 2, 2024 06:34
@SourceOfHTML
Copy link
Contributor Author

SourceOfHTML commented Oct 2, 2024

SORRY sorry that should not have happened, I'll revert it, apologies to all involved parties

nvm I dont know what I'm doing, I feel like anything else I'll do at this point will be another slip-up

@Zireael07
Copy link
Contributor

At this point you've got to close this PR and remake it from scratch, you messed up rebasing

@SourceOfHTML
Copy link
Contributor Author

RIght yes that makes sense, I'll do that at some point

@SourceOfHTML
Copy link
Contributor Author

With the help of some people from the Contributor Chat, I undid the mess, and will make a note to keep my work organized on separate branches. Apologies again, to all involved who had to deal with my nonsense. There was no need to redo the PR tho, so yay for that.

@mhilbrunner
Copy link
Member

Don't worry too much about it, everyone has to learn sometime and Git is a complicated beast. Thanks for contributing.

@akien-mga akien-mga merged commit 5410000 into godotengine:master Oct 2, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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

Successfully merging this pull request may close these issues.

AudioStreamPlayback method _stop() is not called when AudioStreamPlayer.stop() is called
6 participants