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

Project Beats: add skip controls #52343

Merged
merged 7 commits into from
Jul 21, 2023
Merged

Conversation

sanchitmalhotra126
Copy link
Contributor

@sanchitmalhotra126 sanchitmalhotra126 commented Jun 16, 2023

Adds outside of playback skip forward/backwards controls. These advance the playhead forwards or backwards by one measure. Also clicking a measure number or the measures header background will move the playhead to that position outside of playback. To show the current start position, we show the playhead outside of playback in a neutral light color, which switches to the current active yellow when playback starts.

Skip.Controls.Updated.mov

Links

Music Lab Functional Spec

Testing story

Tested locally.

@sanchitmalhotra126 sanchitmalhotra126 requested review from breville, moneppo and a team June 16, 2023 18:16
Copy link
Collaborator

@levadadenys levadadenys left a comment

Choose a reason for hiding this comment

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

LGTM!
Wow, the progress with Music Lab is awesome. Looks like web version of Ableton/FL studio to me (which is awesome 😄) already or at least is getting closer and closer to it

@breville
Copy link
Member

Very cool. Some initial UI thoughts:

  • I really liked the Run button being the full width of the panel. Could we possible put these buttons in the empty space below the triggers?
  • With white backgrounds, these buttons feel more prominent than then Run button. Could we tone them down or consider purple?
  • We should disable them when playing, since they don't do anything.
  • Would it be worth having a third button which jumps the playhead back to the beginning?
  • This exposes the deeper issue that we don't support playing a sample that's not at its beginning. Most noticeably, if you start from one bar into the song, but the song comprises two-bar long samples (which we use a lot), then you don't hear that sound at all. Should we consider updating our audio implementation to support starting a sound from partway through?

@sanchitmalhotra126
Copy link
Contributor Author

  • I really liked the Run button being the full width of the panel. Could we possible put these buttons in the empty space below the triggers?

No strong preference here. I went with the current mockups that have the Run and Skip buttons in one row.

  • With white backgrounds, these buttons feel more prominent than then Run button. Could we tone them down or consider purple?

No strong preference here either. Maybe a more neutral grey?

  • We should disable them when playing, since they don't do anything.

Good call.

  • Would it be worth having a third button which jumps the playhead back to the beginning?

Hmm, yeah that would be helpful. Alternatively, in discussing with @mikeharv another idea could be to have a separate play button that plays from the current location, while "Run" always plays from the beginning?

  • This exposes the deeper issue that we don't support playing a sample that's not at its beginning. Most noticeably, if you start from one bar into the song, but the song comprises two-bar long samples (which we use a lot), then you don't hear that sound at all. Should we consider updating our audio implementation to support starting a sound from partway through?

Yep true, though this would be a little trickier to support. I'd defer to @moneppo on how necessary this feels?

@breville
Copy link
Member

One other thought: should we parameterize the appearance of this feature? It feels like we might want to introduce it at a later point in a tutorial sequence, when things start to get more complex.

@breville
Copy link
Member

breville commented Jun 21, 2023

Two issues we discussed offline:

  • after hitting stop, the highlighted Blockly blocks don't unhighlight.
  • for the disabled buttons, we can try making the existing ones opacity: 50%.

@sanchitmalhotra126
Copy link
Contributor Author

@breville revisiting this PR, addressing the feedback we discussed

  • after hitting stop, the highlighted Blockly blocks don't unhighlight.

this should be fixed now!

  • for the disabled buttons, we can try making the existing ones opacity: 50%.

Done

One other thought: should we parameterize the appearance of this feature? It feels like we might want to introduce it at a later point in a tutorial sequence, when things start to get more complex.

This feature is now hidden behind a config flag (skip-controls=(enabled|disabled)). I've also gone ahead and put keyboard shortcuts behind a config flag as well (keyboard-shortcuts=(enabled|disabled)). Both will default to disabled if not specified.

Comment on lines 89 to 90
{value: 'disabled', description: 'Disable skip controls.'},
{value: 'enabled', description: 'Enable skip controls.'},
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: we use true and false for other values in the menu. enabled and disabled are definitely nice, but should we be consistent with whichever we choose? One factor might be what works best in the context of levelbuilder parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good call, I'll make this true/false for consistency.

Copy link
Member

@breville breville left a comment

Choose a reason for hiding this comment

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

Great feature!

@sanchitmalhotra126 sanchitmalhotra126 merged commit bfca90b into staging Jul 21, 2023
2 checks passed
@sanchitmalhotra126 sanchitmalhotra126 deleted the sanchit/music/skip branch July 21, 2023 19:31
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

3 participants