View Issue Details

IDProjectCategoryView StatusLast Update
0001248OpenMPTFeature Requestpublic2019-09-27 21:13
Reporterdem1 Assigned ToSaga Musix  
PrioritynormalSeverityfeatureReproducibilityN/A
Status resolvedResolutionfixed 
Target VersionOpenMPT 1.29 / libopenmpt 0.5 (goals)Fixed in VersionOpenMPT 1.29 / libopenmpt 0.5 (goals) 
Summary0001248: Relative Chords with notes below the root
Description

Hello,

Currently, when using the Chord Editor with "Relative" base mode, you can only choose chords with notes above the root. I would like to have the option to include notes up to one octave below the root.

Thank you,
Derren

TagsNo tags attached.
Has the bug occurred in previous versions?
Tested code revision (in case you know it)

Activities

Saga Musix

Saga Musix

2019-08-26 19:00

administrator   ~0004028

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?

dem1

dem1

2019-08-30 20:29

reporter   ~0004033

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?

Saga Musix

Saga Musix

2019-08-30 21:11

administrator   ~0004034

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.)

dem1

dem1

2019-09-09 01:29

reporter   ~0004051

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?

StarWolf3000

StarWolf3000

2019-09-09 06:23

reporter   ~0004053

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

Saga Musix

Saga Musix

2019-09-09 10:44

administrator   ~0004054

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.

Saga Musix

Saga Musix

2019-09-09 11:10

administrator   ~0004055

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.

RelativeChords.patch (5,016 bytes)   
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);
 			}
 		}
 	}
RelativeChords.patch (5,016 bytes)   
dem1

dem1

2019-09-16 02:57

reporter   ~0004070

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?

dem1

dem1

2019-09-16 10:59

reporter   ~0004072

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."

dem1

dem1

2019-09-18 03:20

reporter  

1.29.00.28_thicccchordeditor.patch (9,775 bytes)   
Index: mptrack/PatternEditorDialogs.cpp
===================================================================
--- mptrack/PatternEditorDialogs.cpp	(revision 12048)
+++ 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
@@ -891,22 +891,25 @@
 	m_CbnBaseNote.SetItemData(m_CbnBaseNote.AddString(_T("Relative")), MPTChord::relativeMode);
 	AppendNotesToControl(m_CbnBaseNote, NOTE_MIN, NOTE_MIN + 3 * 12 - 1);
 
-	// Minor notes
+	// Chord Factor combo boxes
 	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 if(inotes >= 0)
+		{
+			s = mpt::ToCString(CSoundFile::GetDefaultNoteName(inotes % 12));
+			if(inotes >= 12) s.AppendFormat(_T(" (+%d)"), inotes / 12);
 		} else
 		{
-			s = mpt::ToCString(CSoundFile::GetDefaultNoteName(inotes % 12));
-			if(inotes >= 12)
-				s.AppendFormat(_T(" (+%d)"), inotes / 12);
+			s = mpt::ToCString(CSoundFile::GetDefaultNoteName((24 + inotes) % 12));
+			s.AppendFormat(_T(" (-%d)"), (11 - inotes) / 12);
 		}
-		m_CbnNote1.AddString(s);
-		m_CbnNote2.AddString(s);
-		m_CbnNote3.AddString(s);
+		m_CbnNote1.SetItemData(m_CbnNote1.AddString(s), inotes);
+		m_CbnNote2.SetItemData(m_CbnNote2.AddString(s), inotes);
+		m_CbnNote3.SetItemData(m_CbnNote3.AddString(s), inotes);
 	}
 	// Update Dialog
 	OnChordChanged();
@@ -928,47 +931,67 @@
 {
 	if (wParam != KBDNOTIFY_LBUTTONDOWN) return 0;
 	MPTChord &chord = GetChord();
-	UINT cnote = NOTE_NONE;
-	chord.notes[0] = NOTE_NONE;
-	chord.notes[1] = NOTE_NONE;
-	chord.notes[2] = NOTE_NONE;
-	for(UINT i = 0; i < 2 * 12; i++)
+	int8 newKey = 1 + -2 * 12 + (int8)nKey;
+	//NOTE_NONE is zero, so "C-" or "root note" uses ZERO_SURROGATE.
+	newKey = (newKey == NOTE_NONE) ? ZERO_SURROGATE : newKey;
+
+	//Clear note.
+	for(UINT i = 0; i + 1 < MPTChord::notesPerChord; i++)
 	{
-		if(chord.key == MPTChord::relativeMode)
+		if(chord.notes[i] == newKey)
 		{
-			if(!i) continue;
-		} else
-		{
-			if(i == chord.key % 12u) continue;
+			for(UINT j = i; j + 1 < MPTChord::notesPerChord; j++)
+			{
+				chord.notes[j] = (j + 2 < MPTChord::notesPerChord) ? chord.notes[j + 1] : NOTE_NONE;
+			}
+			OnChordChanged();
+			return 0;
 		}
+	}
 
-		UINT n = m_Keyboard.GetFlags(i);
-		if (i == (UINT)nKey) n = (n) ? 0 : 1;
-		if (n)
+	//Add note.
+	for(UINT i = 0; i + 1 < MPTChord::notesPerChord; i++)
+	{
+		if(chord.notes[i] == NOTE_NONE || i + 2 >= MPTChord::notesPerChord)
 		{
-			if ((cnote < 3) || (i == (UINT)nKey))
-			{
-				UINT k = (cnote < 3) ? cnote : 2;
-				chord.notes[k] = static_cast<BYTE>(i+1);
-				if (cnote < 3) cnote++;
-			}
+			chord.notes[i] = newKey;
+			OnChordChanged();
+			return 0;
 		}
 	}
-	OnChordChanged();
-	return 0;
+
+	//Do nothing.
+	return 1;
 }
 
 
 void CChordEditor::OnChordChanged()
 {
+	int8 tempChord[3];  //scratch copy of chord.notes
 	MPTChord &chord = GetChord();
 	if(chord.key != MPTChord::relativeMode)
 		m_CbnBaseNote.SetCurSel(chord.key + 1);
 	else
 		m_CbnBaseNote.SetCurSel(0);
-	m_CbnNote1.SetCurSel(chord.notes[0]);
-	m_CbnNote2.SetCurSel(chord.notes[1]);
-	m_CbnNote3.SetCurSel(chord.notes[2]);
+	for (UINT i = 0; i + 1 < MPTChord::notesPerChord; i++)
+	{
+		tempChord[i] = chord.notes[i];
+		if(tempChord[i] == ZERO_SURROGATE)
+		{
+			tempChord[i] = 2 * 12;
+		}
+		else if (tempChord[i] == 0)
+		{
+			tempChord[i] = 0;
+		}
+		else
+		{
+			tempChord[i] += 2 * 12;
+		}
+	}
+	m_CbnNote1.SetCurSel(tempChord[0]);
+	m_CbnNote2.SetCurSel(tempChord[1]);
+	m_CbnNote3.SetCurSel(tempChord[2]);
 	UpdateKeyboard();
 }
 
@@ -981,15 +1004,26 @@
 	{
 		note = 0;
 	}
-	for(UINT i = 0; i < 2 * 12; i++)
+	for(INT i = -2 * 12; i < 2 * 12; i++)
 	{
 		uint8 b = CKeyboardControl::KEYFLAG_NORMAL;
-		if(i == note) b = CKeyboardControl::KEYFLAG_REDDOT;
-		else if(chord.notes[0] && i + 1 == chord.notes[0]) b = CKeyboardControl::KEYFLAG_REDDOT;
-		else if(chord.notes[1] && i + 1 == chord.notes[1]) b = CKeyboardControl::KEYFLAG_REDDOT;
-		else if(chord.notes[2] && i + 1 == chord.notes[2]) b = CKeyboardControl::KEYFLAG_REDDOT;
-		m_Keyboard.SetFlags(i, b);
+		if(i + 1 && i + 1 == chord.notes[0])
+			b = CKeyboardControl::KEYFLAG_REDDOT;
+		else if(i + 1 && i + 1 == chord.notes[1])
+			b = CKeyboardControl::KEYFLAG_REDDOT;
+		else if(i + 1 && i + 1 == chord.notes[2])
+			b = CKeyboardControl::KEYFLAG_REDDOT;
+		else if(i == (INT)note)
+			b = CKeyboardControl::KEYFLAG_BRIGHTDOT;
+		m_Keyboard.SetFlags(i + 2 * 12, b);
 	}
+	for(UINT i = 0; i + 1 < MPTChord::notesPerChord; i++)
+	{
+		if(chord.notes[i] == ZERO_SURROGATE)
+		{
+			m_Keyboard.SetFlags(2 * 12 - 1, CKeyboardControl::KEYFLAG_REDDOT);
+		}
+	}	
 	m_Keyboard.InvalidateRect(NULL, FALSE);
 }
 
@@ -1009,11 +1043,18 @@
 {
 	MPTChord &chord = GetChord();
 	int note = m_CbnNote1.GetCurSel();
-	if(note >= 0)
+	if(note < 0)
+		return;
+	if(note == 2 * 12)
 	{
-		chord.notes[0] = (uint8)note;
-		UpdateKeyboard();
+		note = ZERO_SURROGATE;
 	}
+	else if (note)
+	{
+		note -= 2 * 12;
+	}
+	chord.notes[0] = (int8)note;
+	UpdateKeyboard();
 }
 
 
@@ -1021,11 +1062,17 @@
 {
 	MPTChord &chord = GetChord();
 	int note = m_CbnNote2.GetCurSel();
-	if(note >= 0)
+	if(note < 0)
+		return;
+	if(note == 2 * 12)
 	{
-		chord.notes[1] = (uint8)note;
-		UpdateKeyboard();
+		note = ZERO_SURROGATE;
+	} else if(note)
+	{
+		note -= 2 * 12;
 	}
+	chord.notes[1] = (int8)note;
+	UpdateKeyboard();
 }
 
 
@@ -1033,11 +1080,17 @@
 {
 	MPTChord &chord = GetChord();
 	int note = m_CbnNote3.GetCurSel();
-	if(note >= 0)
+	if(note < 0)
+		return;
+	if(note == 2 * 12)
 	{
-		chord.notes[2] = (uint8)note;
-		UpdateKeyboard();
+		note = ZERO_SURROGATE;
+	} else if(note)
+	{
+		note -= 2 * 12;
 	}
+	chord.notes[2] = (int8)note;
+	UpdateKeyboard();
 }
 
 
Index: mptrack/PatternEditorDialogs.h
===================================================================
--- mptrack/PatternEditorDialogs.h	(revision 12048)
+++ mptrack/PatternEditorDialogs.h	(working copy)
@@ -108,6 +108,13 @@
 
 class CChordEditor: public CDialog
 {
+
+public:
+	enum
+	{
+		ZERO_SURROGATE = 0x3E
+	};
+
 protected:
 	CKeyboardControl m_Keyboard;
 	CComboBox m_CbnShortcut, m_CbnBaseNote, m_CbnNote1, m_CbnNote2, m_CbnNote3;
Index: mptrack/TrackerSettings.cpp
===================================================================
--- mptrack/TrackerSettings.cpp	(revision 12048)
+++ 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 12048)
+++ 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 12048)
+++ mptrack/View_pat.cpp	(working copy)
@@ -5193,18 +5193,18 @@
 
 	for(auto cnote : chord.notes)
 	{
-		if(cnote)
+		if(cnote) //Check for NOTE_NONE
 		{
-			ModCommand::NOTE chordNote = key - NOTE_MIN;
+			int32 chordNote = key - NOTE_MIN;
 			if(!relativeMode)
 			{
 				// Only use octave information from the base key
 				chordNote = (chordNote / 12) * 12;
 			}
-			chordNote += cnote;
-			if(specs.HasNote(chordNote))
+			chordNote += (cnote == CChordEditor::ZERO_SURROGATE) ? 0 : (int32)cnote;
+			if(chordNote >= NOTE_MIN && chordNote <= NOTE_MAX && specs.HasNote(static_cast<ModCommand::NOTE>(chordNote)))
 			{
-				outNotes[numNotes++] = chordNote;
+				outNotes[numNotes++] = static_cast<ModCommand::NOTE>(chordNote);
 			}
 		}
 	}
dem1

dem1

2019-09-18 03:20

reporter   ~0004073

Took a stab at it.

Saga Musix

Saga Musix

2019-09-18 20:32

administrator   ~0004074

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

Saga Musix

Saga Musix

2019-09-22 10:23

administrator   ~0004081

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.

Saga Musix

Saga Musix

2019-09-22 13:16

administrator   ~0004082

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.

dem1

dem1

2019-09-22 18:03

reporter   ~0004083

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.
Saga Musix

Saga Musix

2019-09-22 18:19

administrator   ~0004085

Last edited: 2019-09-22 19:27

View 2 revisions

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.

Saga Musix

Saga Musix

2019-09-22 18:49

administrator   ~0004086

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?

Saga Musix

Saga Musix

2019-09-22 19:16

administrator   ~0004087

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

dem1

dem1

2019-09-23 00:56

reporter   ~0004088

Last edited: 2019-09-26 10:14

View 2 revisions

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?
Saga Musix

Saga Musix

2019-09-23 05:57

administrator   ~0004089

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 :)

dem1

dem1

2019-09-25 22:20

reporter   ~0004093

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.

Saga Musix

Saga Musix

2019-09-27 21:13

administrator   ~0004094

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.

Issue History

Date Modified Username Field Change
2019-08-11 18:39 dem1 New Issue
2019-08-26 19:00 Saga Musix Note Added: 0004028
2019-08-30 20:29 dem1 Note Added: 0004033
2019-08-30 21:11 Saga Musix Note Added: 0004034
2019-09-09 01:29 dem1 Note Added: 0004051
2019-09-09 06:23 StarWolf3000 Note Added: 0004053
2019-09-09 10:44 Saga Musix Note Added: 0004054
2019-09-09 11:10 Saga Musix File Added: RelativeChords.patch
2019-09-09 11:10 Saga Musix Note Added: 0004055
2019-09-16 02:57 dem1 Note Added: 0004070
2019-09-16 10:59 dem1 Note Added: 0004072
2019-09-18 03:20 dem1 File Added: 1.29.00.28_thicccchordeditor.patch
2019-09-18 03:20 dem1 Note Added: 0004073
2019-09-18 20:32 Saga Musix Note Added: 0004074
2019-09-22 10:21 Saga Musix Assigned To => Saga Musix
2019-09-22 10:21 Saga Musix Status new => assigned
2019-09-22 10:23 Saga Musix Note Added: 0004081
2019-09-22 13:16 Saga Musix Note Added: 0004082
2019-09-22 13:16 Saga Musix Status assigned => feedback
2019-09-22 13:16 Saga Musix Target Version => OpenMPT 1.29 / libopenmpt 0.5 (goals)
2019-09-22 18:03 dem1 Note Added: 0004083
2019-09-22 18:03 dem1 Status feedback => assigned
2019-09-22 18:19 Saga Musix Note Added: 0004085
2019-09-22 18:49 Saga Musix Note Added: 0004086
2019-09-22 19:16 Saga Musix Status assigned => feedback
2019-09-22 19:16 Saga Musix Note Added: 0004087
2019-09-22 19:27 Saga Musix Note Edited: 0004085 View Revisions
2019-09-23 00:56 dem1 Note Added: 0004088
2019-09-23 00:56 dem1 Status feedback => assigned
2019-09-23 05:57 Saga Musix Note Added: 0004089
2019-09-25 22:20 dem1 Note Added: 0004093
2019-09-26 10:14 Saga Musix Note Edited: 0004088 View Revisions
2019-09-27 21:13 Saga Musix Note Added: 0004094
2019-09-27 21:13 Saga Musix Status assigned => resolved
2019-09-27 21:13 Saga Musix Resolution open => fixed
2019-09-27 21:13 Saga Musix Fixed in Version => OpenMPT 1.29 / libopenmpt 0.5 (goals)