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

Non-Intrusive refactor of play_queued_audio_system() #10910

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

matiqo15
Copy link
Member

@matiqo15 matiqo15 commented Dec 8, 2023

Objective

Solution

  • Use early returns to minimize nesting.
  • Change emitter_translation to use if let instead of map.

@matiqo15 matiqo15 added A-Audio Sounds playback and modification C-Code-Quality A section of code that is hard to understand or change labels Dec 8, 2023
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

LGTM.

Good job in bringing it down to 4 levels (IIRC Clippy doesn't count match arm blocks as blocks). Personally, I would've extracted at least one function, since the function is quite long, and I found an else block like 70 lines after the if started, but we're already at a much better position than main. Also, congratulations on transforming emitter_translation, I wouldn't have ever been able to come up with a similar solution 😃

@Nilirad Nilirad added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 8, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 9, 2023
Merged via the queue into bevyengine:main with commit 1523e8c Dec 9, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Audio Sounds playback and modification C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants