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

MTS "Bulk Tuning Dump" exporter & Korg exporter refactoring #447

Merged
merged 1 commit into from
Nov 26, 2023

Conversation

vsicurella
Copy link
Member

This adds a .syx exporter that adheres to the MIDI Tuning specification of the "Bulk Tuning Dump" SysEx message, and also refactors the Korg exporter to be able to export in different formats associated with different supported synth models.

The Korg exporter now relies on some of the same code as the MTS Exporter since it's really just a wrapper around the same data.

Note: The previous Korg exporter implementation had some math errors affecting the least-signficant byte of the tuning data for each note. I'm not sure the extent of the error (it's a tricky calculation) but I would expect noticeable differences and would encourage users to re-export and re-import their scales.

@vsicurella vsicurella force-pushed the mts-korg-exporters-squashed branch from ed49c41 to 6e0037d Compare July 8, 2023 23:04
type="text"
id="name"
v-model="name"
maxlength="16"
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is enough validation? I'm unable to break it myself, but you could enforce clampName everytime the ref is updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

That must have been an oversight after I realized how to do it for presetIndex. I'm used to using computed properties in Vue2 for this kinda stuff, so I was toying around with it before I figured it out.

@frostburn
Copy link
Member

The commit messages should reference their respective issues #384 and #387 .

@frostburn
Copy link
Member

The sister PR xenharmonic-devs/xen-dev-utils#7 needs to be resolved and published before this PR can be merged.

@vsicurella vsicurella force-pushed the mts-korg-exporters-squashed branch 2 times, most recently from e580dfd to 9b37db1 Compare July 15, 2023 19:48
// const deviceId = ref(0);
// const bank = ref(0); (only for message 0x0804)

const name = ref(clampName(props.scaleName));
Copy link
Member

Choose a reason for hiding this comment

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

This update should hapen every time the modal is opened. Now this line of code basically reads like a statement of intent that masquerades as a computed property.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was able to add a watcher on props to solve this, but I'm not sure if this is the best way to implement input formatting. Do you think there's a better way?
I tried an approach with a writable computed property (something I normally do in BootstrapVue) but couldn't get it to work, plus the Vue3 documentation says that should only be used in rare cases anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's a weird case because you want changes in the scale name to affect the clamped name but not the other way around. I'll check if there's a straightforward solution.

Copy link
Member

Choose a reason for hiding this comment

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

This does what I'm after: Updating the name on the main interface changes what's in the modal, but modifying the modal text input doesn't reflect back on the main interface.

const name = ref("");

watch(
  () => props.scaleName,
  (newValue) => {
    name.value = clampName(newValue);
  },
  { immediate: true }
);

Copy link
Member

Choose a reason for hiding this comment

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

You could attach a watcher to the name ref to do the input formatting, but the currently implemented @input should work too.
@input is already used in the project elsewhere, but for consistency you could refactor the logic into a method so that there's no assignment code inside the template.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks so much! That's close to what I came up with too, good to have some validation on that.

@vsicurella
Copy link
Member Author

The one thing I noticed that I'd rather pass on fully solving for this particular PR is improving the circle consistency of the info-question CSS definition. I noticed on a different computer it doesn't render a perfect circle, I'm guessing because of OS font differences.

@frostburn
Copy link
Member

The one thing I noticed that I'd rather pass on fully solving for this particular PR is improving the circle consistency of the info-question CSS definition. I noticed on a different computer it doesn't render a perfect circle, I'm guessing because of OS font differences.

That's perfectly OK. Simply create a new issue about circle consistency and reference this PR so that we can fix it later.

@vsicurella vsicurella force-pushed the mts-korg-exporters-squashed branch 2 times, most recently from 1cc89e7 to 44ba665 Compare November 25, 2023 20:30
src/assets/base.css Outdated Show resolved Hide resolved
@frostburn
Copy link
Member

The 'OK' button must be fixed. As of now clicking it does nothing.

Other than that, great work!

I won't test the actual files generated as I don't have the hardware. We have two production deployments so Korg owners can do the testing for us.

<template #footer>
<div class="btn-group">
<button
@click="$emit('confirm')"
Copy link
Member

Choose a reason for hiding this comment

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

Still not calling doExport.

@frostburn
Copy link
Member

Seems gnome 🧝 free now. Merging.

@frostburn frostburn merged commit 94fbd62 into main Nov 26, 2023
1 check passed
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.

2 participants