Music: initial progression#50596
Conversation
Very rough initial work, but it does work.
sanchitmalhotra126
left a comment
There was a problem hiding this comment.
Looking good! Some comments but nothing major.
|
|
||
| // A sound picker might want to show the subset of sounds permitted by the | ||
| // progression's currently allowed sounds. | ||
| getAllowedSounds(folderType: string) : Sounds { |
There was a problem hiding this comment.
Should our components be allowed to decide whether to use allowed sounds or not? I wonder if maybe we should just modify the model in place and mark certain sounds/folders allowed: true/false, and use those flags in the other functions here...not sure.
Also should we be calling this in PatternPanel and ChordPanel too?
There was a problem hiding this comment.
I really like the idea of having an immutable library that we initially load, and then being able to dynamically set the allowed sound list as we transition levels. As part of that "functional" philosophy, I like having this transformation function which leaves the library unchanged but still retrieves the filtered set.
I suspect we might end up with places in our code that want access to the unfiltered library, and I think we trust our components to use the allowed set via this function when they should.
It seems most likely we'll want to expand this to patterns and chords too, but I figured we could add them in a follow-up if/when we get the ask. :)
| // Find out from the lab-specific code whether we should be trying to | ||
| // check conditions at the moment. Otherwise, we might get a fail | ||
| // when we shouldn't have even been checking. | ||
| if (!this.validator.shouldCheckConditions()) { |
There was a problem hiding this comment.
Curious about this - if the lab itself is calling updateProgress, can we trust that the lab will only call it when it's appropriate to check conditions?
There was a problem hiding this comment.
It's a great question, and something to keep in mind as Music Lab gets more states, especially for things like loading data, transitioning levels, etc. We could already add a bit of nuance to Music Lab, so that it only calls this when playing, for example, but I think what we have is alright for a first pass.
It seems that if ProgressManager is to remain lab agnostic, we need to ultimately place that responsibility on the caller. I guess we could add some more states to ProgressManager, allowing the caller to give it some context as to when to check state, but I'm not quite sure what they would be yet.
sanchitmalhotra126
left a comment
There was a problem hiding this comment.
Thanks for addressing all the feedback! Just one other question I missed from last time, and a few small nits
| }); | ||
| } | ||
|
|
||
| this.progressManager?.updateProgress(); |
There was a problem hiding this comment.
Do we need to be checking this constantly? Could just this happen onBlockSpaceChange and/or on play?
There was a problem hiding this comment.
I'll adjust it so that it only checks while we are playing, but we need it to check frequently while playing, because the current success criteria are for things actually happening in realtime while the program is running (like, are three sounds playing simultaneously). We could evaluate them in other ways, like examining the program, or examining the timeline, but for tasks such as "trigger a sound using the Beatpad" we'd need to be doing this kind of realtime checking anyway. This is the approach we took for validation in Star Wars & AI for Oceans, too.
This introduces the beginnings of a progression system for Music Lab, allowing us to scaffold the student experience.
Features
Video
music-progression-1.mov
Architecture
The initial implementation was specific to Music Lab, but @sanchitmalhotra126 had good feedback that we could make it more generic, and proposed an architecture which is now in place:
ProgressManagermanages progress, and is lab-agnostic.MusicValidatordoes Music Lab-specific evaluation of progress. It implements the abstractValidatorclass, and is passed intoProgressManager, which knows of nothing more thanValidator.ConditionsCheckeris a helper class which accumulates satisfied conditions, and in this case is used byMusicValidator. Other labs might benefit from it, though there are alternate techniques to validate code, such as direct code examination, which doesn't really need to accumulate satisfied conditions over a period of time.For now,
MusicViewloads the progression data and passes it intoProgressManagerwhen it instantiates that class.MusicViewalso instantiatesMusicValidatorand gives it access to the components it needs to examine progress.Also for now,
Instructionsare aware of the progression, though we might make them agnostic later.Viewing
The progression isn't enabled by default, but can be seen with the
load-progression=trueURL parameter.It does support loading a progression file from S3, but since we haven't put one on there yet, the
local-progression=trueURL parameter will load the built-in file from this commit. Once a file is on S3, specifically https://curriculum.code.org/media/musiclab/music-progression.json (incdo-curriculum/media/musiclab), this option can be omitted.It is also possible to specify an alternate progression, e.g.
?progression=variant, and that variant will be loaded from https://curriculum.code.org/media/musiclab/music-progression-variant.json, allowing us to quickly develop some alternate progressions in parallel.These three new options have also been added to the menu at https://studio.code.org/musiclab/menu.
All that said, the way to see the progression for now is by visiting https://studio.code.org/projectbeats?load-progression=true&local-progression=true.