Definitely an interesting idea but sadly not entirely trivial to implement with the current chord-related code. In particular the visual chord editor will get tricky to use - so far it constructs chords simply from the bottom note to the top note. But what if you click on a note below the current root note - will it become the new root note, or will it become an additional note below the root?

I'd think in that case it should be an additional note below the root - I think it makes sense to have the root note permanently set to C in "Relative" mode, like it is now. I would just prefer that it be the C one octave higher, to allow inversions. That would be consistent with how it works in Absolute mode, I think, since the only way to change the root then is to change the base key.
Are you saying that this would conflict with building chords from the bottom up?
Is all of the relevant chord-related code in PatternEditorDialogs?

Are you saying that this would conflict with building chords from the bottom up?

I'm just not sure if the UI would still work intuitively or how much work it would be to make it intuitive.

Is all of the relevant chord-related code in PatternEditorDialogs?

The chord editor, yes, but there is code outside of that which needs to be touched as well (chord keys are unsigned integers, etc.)

Here's what I'm thinking so far - is this a good approach?

• Make the keyboard three octaves long. In CChordEditor::OnInitDialog(), change m_Keyboard.Init(m_hWnd, 2) to m_Keyboard.Init(m_hWnd, 3). Change the for loop to run from inotes=-13 to <24; change the inotes < 0 condition to inotes <= -13.
• Indicate visually which note is the root note - maybe write "X" inside the root note's reddot, or 1, 2, and 3 in the other notes' reddot, or draw the root note with a different coloured dot. I have no idea how much work it took to make the sample map editor do this. Hopefully this can be done by changing only CChordEditor.
• To this end, change the length of the for loop in CChordEditor::UpdateKeyboard to 3 * 12. Change note = 0 to note = 12. Below that, change if(i == note) b = CKeyboardControl::KEYFLAG_REDDOT; to KEYFLAG_BRIGHTDOT, and rearrange the if... else if so that the base note is checked last (if a user clicks the C- key while the editor is in relative mode there should be a red dot, otherwise it should be the root note colour.)
• Leave the chord keys unsigned, so that a 0 indicates a note one octave below the root in relative mode, and the C one octave below the current note in absolute mode. Change the default chords starting on line 386 of TrackerSettings::TrackerSettings(conf) to be 12 higher (5, 8, 11 become 17, 20, 23.)
• In CViewPattern::ConstructChord(), subtract 12 in the definition of chordNote. Check for underflows inside the for loop - if a special note were intended, like when TempStopNote calls this function, it would get caught before the loop. If there is an underflow, do not include the note.
• Change the length of the for loop in CChordEditor::OnKeyboardNotify to 3 * 12.

Note that I haven't been able to make or test any of these changes yet. Do I need to pay for Visual Studio in order to compile OpenMPT?

There's a Community Edition since version 2015, so just download and install that one. It is not recommended to install the Express editions.

Visual Studio 2017 or 2019 Community edition will do, but you need to make sure that you install MFC (it's not checked by default in the installer).

Some parts of your approach are a bit flawed, in the sense that they will break already saved chords (the whole adding / subtracting 12 thing), this is why I wanted to actually have negative offsets for chords in the MPTChord struct. If you are interested, I can send you what I have done so far, which is basically everything except for the chord editor. Then you could take on from there.

Here is my current progress, which should leave all the remaining work to be in the chord editor. I have chosen a -2/+2 octave range for symmetry reasons.

Index: mptrack/PatternEditorDialogs.cpp
===================================================================
--- mptrack/PatternEditorDialogs.cpp	(revision 12033)
+++ mptrack/PatternEditorDialogs.cpp	(working copy)
@@ -880,7 +880,7 @@
 	CMainFrame *pMainFrm;
 
 	CDialog::OnInitDialog();
-	m_Keyboard.Init(m_hWnd, 2);
+	m_Keyboard.Init(m_hWnd, 4);
 	pMainFrm = CMainFrame::GetMainFrame();
 	if (!pMainFrm) return TRUE;
 	// Fills the shortcut key combo box
@@ -893,20 +893,20 @@
 
 	// Minor notes
 	CString s;
-	for (int inotes=-1; inotes<24; inotes++)
+	for (int inotes = -25; inotes < 24; inotes++)
 	{
-		if(inotes < 0)
+		if(inotes == -25)
 		{
 			s = _T("--");
 		} else
 		{
 			s = mpt::ToCString(CSoundFile::GetDefaultNoteName(inotes % 12));
-			if(inotes >= 12)
-				s.AppendFormat(_T(" (+%d)"), inotes / 12);
+			if(inotes < 0 || inotes >= 12)
+				s.AppendFormat(_T(" (%c%d)"), inotes < 0 ? '-' : '+', inotes / 12);
 		}
-		m_CbnNote1.AddString(s);
-		m_CbnNote2.AddString(s);
-		m_CbnNote3.AddString(s);
+		m_CbnNote1.SetItemData(m_CbnNote1.AddString(s), inotes);
+		m_CbnNote1.SetItemData(m_CbnNote2.AddString(s), inotes);
+		m_CbnNote1.SetItemData(m_CbnNote3.AddString(s), inotes);
 	}
 	// Update Dialog
 	OnChordChanged();
@@ -932,11 +932,11 @@
 	chord.notes[0] = NOTE_NONE;
 	chord.notes[1] = NOTE_NONE;
 	chord.notes[2] = NOTE_NONE;
-	for(UINT i = 0; i < 2 * 12; i++)
+	for(UINT i = 0; i < 4 * 12; i++)
 	{
 		if(chord.key == MPTChord::relativeMode)
 		{
-			if(!i) continue;
+			if(i == 2 * 12) continue;
 		} else
 		{
 			if(i == chord.key % 12u) continue;
@@ -949,7 +949,7 @@
 			if ((cnote < 3) || (i == (UINT)nKey))
 			{
 				UINT k = (cnote < 3) ? cnote : 2;
-				chord.notes[k] = static_cast<BYTE>(i+1);
+				chord.notes[k] = static_cast<int8>(i + 1);
 				if (cnote < 3) cnote++;
 			}
 		}
Index: mptrack/TrackerSettings.cpp
===================================================================
--- mptrack/TrackerSettings.cpp	(revision 12033)
+++ mptrack/TrackerSettings.cpp	(working copy)
@@ -393,15 +393,15 @@
 		if(ichord < 12)
 		{
 			// Major Chords
-			Chords[ichord].notes[0] = (uint8)(ichord+5);
-			Chords[ichord].notes[1] = (uint8)(ichord+8);
-			Chords[ichord].notes[2] = (uint8)(ichord+11);
+			Chords[ichord].notes[0] = (int8)(ichord+5);
+			Chords[ichord].notes[1] = (int8)(ichord+8);
+			Chords[ichord].notes[2] = (int8)(ichord+11);
 		} else if(ichord < 24)
 		{
 			// Minor Chords
-			Chords[ichord].notes[0] = (uint8)(ichord-8);
-			Chords[ichord].notes[1] = (uint8)(ichord-4);
-			Chords[ichord].notes[2] = (uint8)(ichord-1);
+			Chords[ichord].notes[0] = (int8)(ichord-8);
+			Chords[ichord].notes[1] = (int8)(ichord-4);
+			Chords[ichord].notes[2] = (int8)(ichord-1);
 		}
 	}
 
@@ -1238,9 +1238,9 @@
 			if((chord & 0xFFFFFFC0) || (!chords[i].notes[0]))
 			{
 				chords[i].key = (uint8)(chord & 0x3F);
-				chords[i].notes[0] = (uint8)((chord >> 6) & 0x3F);
-				chords[i].notes[1] = (uint8)((chord >> 12) & 0x3F);
-				chords[i].notes[2] = (uint8)((chord >> 18) & 0x3F);
+				chords[i].notes[0] = static_cast<int8>(((chord >> 6) & 0x3F) | (0xC0 * ((chord >> 11) & 1)));
+				chords[i].notes[1] = static_cast<int8>(((chord >> 12) & 0x3F) | (0xC0 * ((chord >> 17) & 1)));
+				chords[i].notes[2] = static_cast<int8>(((chord >> 18) & 0x3F) | (0xC0 * ((chord >> 23) & 1)));
 			}
 		}
 	}
@@ -1251,7 +1251,7 @@
 {
 	for(std::size_t i = 0; i < std::size(chords); i++)
 	{
-		int32 s = (chords[i].key) | (chords[i].notes[0] << 6) | (chords[i].notes[1] << 12) | (chords[i].notes[2] << 18);
+		int32 s = (chords[i].key) | ((chords[i].notes[0] & 0x3F) << 6) | ((chords[i].notes[1] & 0x3F) << 12) | ((chords[i].notes[2] & 0x3F) << 18);
 		mpt::ustring note = mpt::format(U_("%1%2"))(mpt::ustring(NoteNamesSharp[i % 12]), i / 12);
 		conf.Write<int32>(U_("Chords"), note, s);
 	}
Index: mptrack/TrackerSettings.h
===================================================================
--- mptrack/TrackerSettings.h	(revision 12033)
+++ mptrack/TrackerSettings.h	(working copy)
@@ -193,7 +193,7 @@
 	};
 
 	uint8 key;			// Base note
-	uint8 notes[3];		// Additional chord notes
+	int8 notes[3];		// Additional chord notes
 };
 
 using MPTChords = std::array<MPTChord, 3 * 12>;	// 3 octaves
Index: mptrack/View_pat.cpp
===================================================================
--- mptrack/View_pat.cpp	(revision 12033)
+++ mptrack/View_pat.cpp	(working copy)
@@ -5195,7 +5195,7 @@
 	{
 		if(cnote)
 		{
-			ModCommand::NOTE chordNote = key - NOTE_MIN;
+			int32 chordNote = key - NOTE_MIN;
 			if(!relativeMode)
 			{
 				// Only use octave information from the base key
@@ -5202,9 +5202,9 @@
 				chordNote = (chordNote / 12) * 12;
 			}
 			chordNote += cnote;
-			if(specs.HasNote(chordNote))
+			if(chordNote >= NOTE_MIN && chordNote <= NOTE_MAX && specs.HasNote(static_cast<ModCommand::NOTE>(chordNote)))
 			{
-				outNotes[numNotes++] = chordNote;
+				outNotes[numNotes++] = static_cast<ModCommand::NOTE>(chordNote);
 			}
 		}
 	}

I've made a little progress on this, but I've gotten snagged: the chords now and the chords with this new setup are incompatible.

Currently NOTE_NONE (=0) is used when a chord has fewer than 4 notes, but allowing chord.note

We could decide on some arbitrary number (0xFA, say) so that when nKey = 24 is passed to CChordEditor::OnKeyboardNotify, chord.note[k] is set to 0xFA, and when CViewPattern::ConstructChord checks cnote and sees it's 0xFA, it treats it like a C- (or the root note, if in relative mode). Would this be acceptable?

Oops, that should read "Currently NOTE_NONE (=0) is used when a chord has fewer than 4 notes, but now that chord.note consists of signed integers, 0 should actually be a valid note."

Took a stab at it.

I'll hopefully have time during the weekend to have a look at it.

There's still a lot of cleanup to be done (we can move the surrgate replacement to loading/saving the settings, which avoids lots of confusing branches in the rest of the code), but nice work nevertheless. I think what bothers me the most really (with the Chord Editor in general, not your patch) is the unintuitive behaviour of the keyboard map. I think it would make more sense if you could drag notes around rather than clicking to replace the last note. I think I will try to incorporate that.

Alright, I think this is done in r12078 / OpenMPT 1.29.00.31. There are several changes in addition to the two extra ocatves:

• The interaction model has changed: Clicking to add a note only works as long as there is an unused note slot in the chord. Otherwise, click-and-drag to arrange existing notes.
• The chord editor is now resizable.

Let me know what you think.

It's great! My notes:

• It didn't break any of my chords from 1.28.06, and the chord editor is still working correctly.
• I like noNote as a part of MPTChord, makes everything way less confusing. I'm confused by noteType, though. Would that type ever need to change from int8, and if it does, wouldn't noNote and Save/LoadChords also need to change?
• I like the click-and-drag very much, great idea. Love having the ability to double notes without using the combo boxes.
• I think if a chord note is selected to coincide with the root note, there should be some visual indication of this on the keyboard. My attempt just used a red circle, but maybe there's a third colour available?
• Is the current way of enabling Relative Mode intuitive enough? I think the text gives a good hint, but I don't know whether a new user would consider that hard to find.
• There's only one extra octave? It's all I wanted, but the revision notes and your comment say two and I just want to make sure you didn't mean to add two.
• Unrelated, but while looking over the diffs, I noticed there is a CSplitKeyboardSettings and a CSplitKeyboadSettings, AND the keyboard shortcut is the only way to open that dialog.

I like noNote as a part of MPTChord, makes everything way less confusing. I'm confused by noteType, though. Would that type ever need to change from int8, and if it does, wouldn't noNote and Save/LoadChords also need to change?

I mostly do this so that, if it ever has to change, it doesn't have to updated in several places. Of course there are still lots of things to be changed if that ever happens, but at least we don't have to change the type e.g. in the std::sort comparator in the chord editor. Generally I like types that are tied to a specific use case. Want to find all declarations of chord notes? Search for NoteType. With int8, you couldn't do that.

I think if a chord note is selected to coincide with the root note, there should be some visual indication of this on the keyboard. My attempt just used a red circle, but maybe there's a third colour available?

There currently isn't but it could be added - or alternatively we could show two dots. Both would have to be implemented but it should be fairly easy.

Is the current way of enabling Relative Mode intuitive enough? I think the text gives a good hint, but I don't know whether a new user would consider that hard to find.

Honestly the whole chord business is all but intuitive. :) I have considered that having relative mode turn on by default for all preset chords (i.e. the ones available on a fresh install) might be the better choice.

There's only one extra octave? It's all I wanted, but the revision notes and your comment say two and I just want to make sure you didn't mean to add two.

Whoops, good catch, I was playing around with the constants to make sure that they work and forgot to undo that.

Unrelated, but while looking over the diffs, I noticed there is a CSplitKeyboardSettings and a CSplitKeyboadSettings, AND the keyboard shortcut is the only way to open that dialog.

Another nice catch. :D Will be renamed. But no, this dialog can also be invoked from the Edit menu.

Alternatively, maybe we should simply forbid sharing the same key between the root and other notes, or is there a good reason why this should be a feature?

As an experiment, I modified the code to two draw dots. Let's see where this goes.

I mostly do this so that, if it ever has to change, it doesn't have to updated in several places. Of course there are still lots of things to be changed if that ever happens, but at least we don't have to change the type e.g. in the std::sort comparator in the chord editor. Generally I like types that are tied to a specific use case. Want to find all declarations of chord notes? Search for NoteType. With int8, you couldn't do that.

Yes, I see how that can be handy, thanks!

There currently isn't but it could be added - or alternatively we could show two dots. Both would have to be implemented but it should be fairly easy.

I think the two dots make it clearer what's going on. I was worried about there not being enough room for both, but I tried resizing the editor and changing the resolution, and nothing unexpected happens when the top of a dot is taller than the key. Interestingly, the dots for a few white keys have different sizes (not an issue for me, I only noticed after some very prolonged staring).

Honestly the whole chord business is all but intuitive. :) I have considered that having relative mode turn on by default for all preset chords (i.e. the ones available on a fresh install) might be the better choice.

Quite possibly, yes. Not sure which chords would make the most sense. I suggest the bottom row remains unassigned by default, so as to avoid any conflict with the default Paste Flood keybinding.

But no, this dialog can also be invoked from the Edit menu.

Lmao, yep, I'll try looking for things next time.

Alternatively, maybe we should simply forbid sharing the same key between the root and other notes, or is there a good reason why this should be a feature?

No good reasons from me, besides the fact that it's already in there. I suppose it could be used for playing several instruments at once, or the same instrument twice with different filter settings, or having a "pedal point" chord that doubles the root for emphasis, but I myself have literally never done this. It's a feature only for people who edit harmonies live using just their computer keyboard.

Some possible issues that depend on how this restriction is implemented:

• As stated, this is already a feature, so what happens if someone previously had such a chord saved?
• What happens if you try and enter a forbidden chord using one of the four combo boxes?
• What happens if you try and drag a note onto/across the root key?
• Can a chord have two of the same note, provided neither is the root note?

If we only allow unique notes, I would only update the notes in the editor, so anything you previously saved would remain intact until you edit it.

BTW: The issue tracker uses Markdown, not HTML, so the correct way to quote is using the > character :)

If we only allow unique notes, I would only update the notes in the editor, so anything you previously saved would remain intact until you edit it.

Oh, yep, that would work. I am fine either way.

I think I'll just leave it as-is for now. Any considerations for better default chords or similar stuff can be discussed in a separate issue.