add velocity editor and asset controls to piano roll - #21
Open
IanMatthewHuff wants to merge 1 commit into
Open
Conversation
IanMatthewHuff
added
the
pxt-review-ops/comparison-pr
There was a problem hiding this comment.
Pull request overview
This PR extends the webapp piano roll editor with asset-editor chrome (name/done controls, editor/my-assets toggle), adds a velocity editor UI, and introduces backwards-compatible velocity encoding/decoding in the song binary format, with simulator playback updated to apply velocity.
Changes:
- Added velocity support end-to-end: editor state →
pxt.assets.musicsong model → song buffer encode/decode → simulator sequencer volume scaling. - Enhanced piano roll UI with a measure header timeline and an optional velocity editor panel, plus updated theming/CSS.
- Integrated piano roll into the existing
ImageFieldEditorasset editor wrapper via a neweditorTypeprop.
Show a summary per file
| File | Description |
|---|---|
| webapp/src/components/PianoRollFieldEditor.tsx | New PianoRollAssetEditor wrapper to host piano roll inside the shared asset editor frame. |
| webapp/src/components/pianoRoll/workspaceBackground.tsx | Generate workspace background SVG using theme-provided colors. |
| webapp/src/components/pianoRoll/Workspace.tsx | Sync scroll with measure header / velocity editor and adjust gesture scrolling behavior. |
| webapp/src/components/pianoRoll/VelocityEditor.tsx | New velocity slider UI and note highlighting behavior. |
| webapp/src/components/pianoRoll/types.ts | Add velocity to note events; convert to/from pxt.assets.music.Song; add note-event update helpers. |
| webapp/src/components/pianoRoll/PianoRoll.tsx | Plumb velocity editor visibility, selected track, and asset name through state + callbacks; add measure header + edit controls. |
| webapp/src/components/pianoRoll/MeasureHeader.tsx | New timeline header component that renders measure numbers. |
| webapp/src/components/pianoRoll/Header.tsx | Add UI toggle for velocity editor. |
| webapp/src/components/pianoRoll/FieldEditor.tsx | Persist selected track + velocity editor visibility; propagate asset name changes into meta. |
| webapp/src/components/pianoRoll/context.tsx | Add theme fields for workspace colors (read from CSS variables). |
| webapp/src/components/ImageFieldEditor.tsx | Replace isMusicEditor with editorType and add piano-roll editor support. |
| webapp/src/blocklyFieldView.tsx | Route field editor injection to the new editorType path for piano roll. |
| webapp/src/assetEditor.tsx | Update asset editor to use editorType instead of isMusicEditor. |
| theme/piano-roll/piano-roll.less | Add/update variables and styles (measure header, velocity editor panel, new blue theme). |
| pxtsim/sound/song.ts | Decode appended velocity sections after track data. |
| pxtsim/sound/sequencer.ts | Apply per-note-event velocity to playback volume. |
| pxtlib/music.ts | Encode/decode appended velocity sections; document updated binary format. |
| localtypings/pxtmusic.d.ts | Add optional velocity to pxt.assets.music.NoteEvent. |
Copilot's findings
Comments suppressed due to low confidence (1)
webapp/src/components/pianoRoll/FieldEditor.tsx:42
getValueassumesassetis defined and accessesasset.metawithout optional chaining. IfgetValueis called beforeinit(or after a reset), this will throw. Useasset?.meta(or guardassetearly) when building the updatedmetaobject.
- Files reviewed: 17/18 changed files
- Comments generated: 6
Comment on lines
+115
to
+118
| const theme = usePianoRollTheme(); | ||
|
|
||
| const description = lf("Change velocity for notes at tick {0}", tick); | ||
| const fill = (velocity / 128) * 100 + "%"; |
Comment on lines
+39
to
+53
| const highlightTick = (tick: number) => { | ||
| if (tick !== highlightedTick) { | ||
| if (highlightedTick) { | ||
| const previousEvents = offsets.find(n => n.start === highlightedTick); | ||
| if (previousEvents) { | ||
| for (const note of previousEvents.events) { | ||
| const el = document.getElementById(`note-${note.id}`); | ||
| if (el) { | ||
| el.classList.remove("highlighted"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| setHighlightedTick(tick); | ||
| const currentEvents = offsets.find(n => n.start === tick); |
Comment on lines
+40
to
+54
| const measureScroller = document.getElementById("measure-header"); | ||
| const velocityEditor = document.getElementById("velocity-editor"); | ||
|
|
||
| const changeHorizontalScroll = (delta: number) => { | ||
| const scroll = gestureState.current.startScrollX - delta; | ||
| if (horizontalScroller) { | ||
| horizontalScroller.scrollLeft = scroll; | ||
| } | ||
| if (measureScroller) { | ||
| measureScroller.scrollLeft = scroll; | ||
| } | ||
| if (velocityEditor) { | ||
| velocityEditor.scrollLeft = scroll; | ||
| } | ||
| } |
Comment on lines
95
to
+96
| stopPlayback(); | ||
| fireStateChange({ asset, undoStack: initialUndoStack || [], redoStack: initialRedoStack || [], selectedTrack: initialSelectedTrack || song.tracks[0].id, velocityEditorVisible: initialVelocityEditorVisible || false }) |
| * 0 track id | ||
| * 1...velocities | ||
| * | ||
| * velocty |
| import { SoundEffectEditor } from "./components/soundEffectEditor/SoundEffectEditor"; | ||
| import { AssetFilePicker } from "./components/AssetFilePicker"; | ||
| import { PianoRollFieldEditor } from "./components/pianoRoll/fieldEditor"; | ||
| import { PianoRollFieldEditor } from "./components/pianoRoll/FieldEditor"; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
more work on the piano roll editor!
changes:
velocity support involves a change to the song format, but it's backwards compatible. the new data is simply tacked on to the end of the song buffer after all of the track data (and is completely optional). this new format will also work just fine with the old player code; the velocity values will just get ignored.
it should be completely safe to open a song from the old song editor in the piano roll editor, but going the other direction might cause you to lose velocity data so it's not recommended. i might add a warning dialog at some point to prevent this from accidentally happening.
i will eventually have another PR in common-packages that adds velocity support to the hardware sequencer implementation. again, it'll be 100% backwards compatible with existing songs.
Mirrored from upstream PR:
https://github.com/microsoft/pxt/pull/11288Created automatically by pxt-review-ops for code-review agent comparison.
(URL wrapped in a code span so GitHub does not create a cross-reference on the upstream timeline.)