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

Add loading of user-defined scales/chords #5204

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Veratil
Copy link
Contributor

@Veratil Veratil commented Sep 26, 2019

Enhancement for #241.

This will look for a notes.xml file in the default working directory. If found, it will load those chords and scales instead of the default ones. I decided to load only one or the other because it gives the option (currently requiring manual moving/deleting/updating/whatever of the file) of changing out XML files in case a user wanted to only see certain chords/scales for a project.

Possible future enhancement: Link projects to custom scale/chord files.

The XML file is laid out as so:

<notes>
  <chords>
    <chord name="Major">
      <semiTone key="0"/>
      ...
    </chord>
    ...
  </chords>
  <scales>
    <scale name="Major">
      <semiTone key="0"/>
      ...
    </scale>
    ...
  </scales>
</notes>

As the current chords/scales are set with the equivalent to pitch class integer notation the keys follow the same.

There is a little leeway with getting the <semiTone> nodes as it doesn't actually look for the node name, so technically as long as the key attribute is there it could be called anything.


Additionally, this PR "fixes" the Chord stacking & arpeggio tab to only list actual chords in the Stacking combobox, instead of including scales as well. I doubt anyone wants to hit all the notes of the scale at the same time. I left the Arpeggio combobox to include them though.

Another slight change is to the octave chord to actually play the next octave up (I don't see the point of having it if it's just the note you select). If this is incorrect then I can revert that line.

lmms-user-chords

lmms-user-scales

@LmmsBot
Copy link

LmmsBot commented Oct 3, 2019

Downloads for this pull request

Generated by the LMMS pull requests bot.
SHA: f7cbc45

@LMMS LMMS deleted a comment from LmmsBot Oct 3, 2019
@Veratil Veratil added needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Dec 7, 2019
@PhysSong
Copy link
Member

to the octave chord to actually play the next octave up (I don't see the point of having it if it's just the note you select)

I think the real problem is the misleading name. IMO it should be none(octave) because you can use the chord(?) to play with notes with octave intervals.

@zonkmachine zonkmachine self-requested a review April 30, 2020 07:57
@zonkmachine
Copy link
Contributor

Completely missed this PR. Sorry!

bool isScale() const
{
return size() > 6;
}

Old way... Yeah, this was maybe a bit too optimistic...

I thought I could just restart the failing Travis build job and an AppImage would magically appear somewhere. That assumption was incorrect.

@zonkmachine
Copy link
Contributor

First impressions.

  • I'm now seeing chords and scales where they're supposed to be. Thanks!
  • I managed to get a closing tag wrong at my first attempt. It would be good with some kind of warning that a failed attempt to load notes.xml had occurred.
  • I think it's better to load user scales on top of or after the default ones. I suggest to have the first entry in the chords/scales tab be a directory to user files if any are present.
  • Maybe add ~/lmms/notes directory, either by default or just looking for its presence, where you can arrange your material.

@Veratil
Copy link
Contributor Author

Veratil commented May 2, 2020

I managed to get a closing tag wrong at my first attempt. It would be good with some kind of warning that a failed attempt to load notes.xml had occurred.

I might change to JSON with the recent idea for switching over. It would be a lot easier to edit.

For instance

{
  "scales": [
    { "name": "Major", "keys": [0, 2, 4, 5, 7, 9, 11] }
  ],
  "chords": [
    { "name": "Major", "keys": [0, 4, 7] }
  ]
}

I think it's better to load user scales on top of or after the default ones. I suggest to have the first entry in the chords/scales tab be a directory to user files if any are present.

I tossed the idea around, but adding to the end of a long list would be annoying. My ideal solution is the idea of attaching a scales/chords file to project files, so they're auto loaded per project. There's a few ways we can handle this really. This PR is a stepping stone to get it in first! 😁

Maybe add ~/lmms/notes directory, either by default or just looking for its presence, where you can arrange your material.

Yeah, like above there's a few ideas we can move to from this. I'd like some more feedback from people who want to use it though.

@dan-giddins
Copy link

@Veratil are you still working on this one? If not, I am happy to take it on...

I agree with the move to JSON. I think its a much more concise way of displaying this information.

I would argue 'keys' should be renamed to 'notes' (i.e. notes of a scale) as a scale's 'key' is something different (the tonic root of a scale). In this case, you are defining a generic set of numerical values that make up the scale.

@Veratil
Copy link
Contributor Author

Veratil commented Jan 31, 2021

@Veratil are you still working on this one? If not, I am happy to take it on...

I agree with the move to JSON. I think its a much more concise way of displaying this information.

I would argue 'keys' should be renamed to 'notes' (i.e. notes of a scale) as a scale's 'key' is something different (the tonic root of a scale). In this case, you are defining a generic set of numerical values that make up the scale.

I put it aside for a while to work on other things, this was from when I was early learning the codebase. If you'd like to fork my branch and make a PR against it I'd be happy to merge them in here, too.

@dan-giddins dan-giddins self-assigned this Jan 31, 2021
@luzpaz
Copy link
Contributor

luzpaz commented Aug 22, 2023

Anyone want to pick up the mantle on this one ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants