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

Music: use pack bpm/key after choosing #57913

Merged
merged 2 commits into from Apr 12, 2024

Conversation

sanchitmalhotra126
Copy link
Contributor

@sanchitmalhotra126 sanchitmalhotra126 commented Apr 9, 2024

Warning!!

The AP CSP Create Performance Task is in progress. The most critical dates are from April 3 - April 30, 2024. Please consider any risk introduced by this PR that could affect our students taking AP CSP. Code.org students taking AP CSP primarily use App Lab for their Create Task, however a small percent use Game Lab. Carefully consider whether your change has any risk of alterering, changing, or breaking anything in these two labs. Even small changes, such as a different button color, are considered significant during this time period. Reach out to the Student Learning team or Curriculum team for more details.

Description

Use the BPM and key from the selected pack once chosen. We'll read the bpm/key from the folder directly, but if it's not specified, we'll use the bpm/key from the first sound in the pack that has the relevant information.

Links

https://codedotorg.atlassian.net/browse/LABS-675

Testing story

Tested with standalone projects, with and without restricted packs, and on the onboarding flow.

@sanchitmalhotra126 sanchitmalhotra126 requested review from breville and a team April 9, 2024 22:14
return this.bpm;
}
const folder = this.getFolderForFolderId(this.currentPackId);
// Read BPM from the folder, or the first sound that has a BPM if not present.
Copy link
Member

Choose a reason for hiding this comment

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

super nit: "if not present" refers to "the folder"?

@breville
Copy link
Member

breville commented Apr 9, 2024

We'll read the bpm/key from the folder directly, but if it's not specified, we'll use the bpm/key from the first sound in the pack that has the relevant information.

Do we need bpm/key in individual sounds for any other reason? Curious whether it would be sufficient to just use the bpm/key from the folder, and to otherwise use the default. This could keep the library format simple.

@sanchitmalhotra126
Copy link
Contributor Author

Do we need bpm/key in individual sounds for any other reason? Curious whether it would be sufficient to just use the bpm/key from the folder, and to otherwise use the default. This could keep the library format simple.

Currently we do read bpm/key for each individual sound when queueing samples, to determine what pitch/bpm offset to apply for each sound. Though, we could definitely instead just read the bpm/key from the sound's folder instead if that's easier to author. I guess the only question is, would we ever have a sound pack with a mix of keys/bpms?

@sanchitmalhotra126 sanchitmalhotra126 merged commit 8200906 into staging Apr 12, 2024
2 checks passed
@sanchitmalhotra126 sanchitmalhotra126 deleted the sanchit/music-artist-bpm-key branch April 12, 2024 21:54
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.

None yet

2 participants