Adding click and play note for Lattices #701
Conversation
|
@frostburn would you mind reviewing this one? Thanks |
|
The issues are poorly organized. Playing notes by clicking is mentioned here in comment in a TODO list: #38 The main reason playing hasn't been implemented is the ambiquity of the equave. Shepard tones could solve that issue, but come with their own set of issues. Let's see what you got here... |
src/components/GridLattice.vue
Outdated
| }>() | ||
| type TouchEventCallback = (event: TouchEvent, index: number) => void |
There was a problem hiding this comment.
These types are repeated multiple times. Make a src/types.ts or similar so that they can be tweaked from a single location in the future if needed.
|
Stuff I noticed
Nice work overall! |
|
Please make an issue about the refactoring if you're not going to do it here. |
|
@wilckerson What's the status of this PR? Just let us know if you don't have time to work on it and I can tidy it up. |
|
@frostburn Sorry for taking so long, I can work on those fixes in the following days. But thank you for offering help =) |
No problem. Thanks for keeping us in the loop. |
Fix PR feedbacks
wilckerson
force-pushed
the
lattice-click-support
branch
from
17d59f9 to
d4bfe71
Compare
| onMounted(() => { | ||
| window.addEventListener('mouseup', onWindowMouseUp) | ||
| }) | ||
| const playNote = usePlayNote(props.noteOn); |
There was a problem hiding this comment.
All the play note logic is now refactored inside this hook. It is referenced here and other components.
| @@ -15,9 +16,6 @@ const scale = useScaleStore() | |||
| const baseIndex = computed( | |||
| () => scale.scale.baseMidiNote + scale.equaveShift * scale.scale.size + scale.degreeShift | |||
| ) | |||
|
|
|||
| type NoteOff = () => void | |||
| type NoteOnCallback = (index: number) => NoteOff | |||
There was a problem hiding this comment.
Removing this in favor of the already existing type inside the xen-midi.
|
@frostburn I have handled your feedback and you may close this PR. Sorry for taking so long.
|
| import { LEFT_MOUSE_BTN } from '@/constants' | ||
| import type { NoteOff, NoteOnCallback } from 'xen-midi' | ||
|
|
||
| export function usePlayNote(noteOn: NoteOnCallback) { |
There was a problem hiding this comment.
Semantically this appears to be better implemented as a class.
| stroke-width="1" | ||
| pointer-events="none" | ||
| /> | ||
| <rect x="100" y="100" width="98" height="98" rx="5" ry="5" :fill="noteOffFill" stroke="lightslategray" |
There was a problem hiding this comment.
While stuff like this arguably looks better, we use automatic formatting so just do npm run format please. If you strongly disagree with the formatter make a separate PR tweaking its settings.
|
Clicking on a lattice node and dragging away enters a buggy mode where the note plays when you hover over the node again. Clicking on the node when in this buggy state makes the note play indefinitely. |
|
The frequencies associated with a node are incorrect when using lattice permutations such as created using Generator sequence and pressing the Lattice button: https://sw3.lumipakkanen.com/scale/_1if4acyL |
|
Cycles and 3D lattice options were added recently? Are you going to add functionality for them too? |
| @@ -4,6 +4,7 @@ import VirtualPiano from '@/components/VirtualPiano.vue' | |||
| import { useStateStore } from '@/stores/state' | |||
| import { useScaleStore } from '@/stores/scale' | |||
| import { computed } from 'vue' | |||
| import type { NoteOnCallback } from 'xen-midi' | |||
|
|
|||
| defineProps<{ | |||
| noteOn: NoteOnCallback | |||
There was a problem hiding this comment.
This prop now has the incorrect type (wrong number of arguments). You need to modify App.vue too.
|
Alright. It sounds good. Let me see what I can do. |
This PR comes from a feature that I miss, which I didn't find an open issue, where the Lattices could play the notes by clicking/touching them. The feature was added to the Lattice node and label, helping with the click region that may become small on mobile phones.
The logic used to handle the click/touch was the same as
VirtualPiano.vue. I think it could be extracted in a helper function and refactored to all places to use the same underlying implementation. However, it can be done in another PR to avoid this one getting bigger.Thanks.