Ghost notes for the automation editor

[DanielKauss]

This PR adds the ability to set ghost notes in the automation editor. Closes #4628 and relates to #1959.
It basically mirrors the functionality of the piano roll, except that if you change the midi clip, the automation notes also update, as discussed in #4628.
The vertical scaling is based on the minimum and maximum keys of the clip. The notes are represented only as squares, as I thought it fits better with the style of the editor, however this is subject to change.

This is how it looks at the moment:

If you have any feedback I will take it into account.

[qnebra]

Must have feature for anyone who wants to write orchestral music in lmms in any meaningful way. I guess it would be useful for basically everyone. What bothers me here is a rendering, having melody in 1,5 whole tone (C#, D# and E) rendered on entire height of automation editor looks strange. Timing and lenght of notes are right.

[DanielKauss]

Must have feature for anyone who wants to write orchestral music in lmms in any meaningful way. I guess it would be useful for basically everyone. What bothers me here is a rendering, having melody in 1,5 whole tone (C#, D# and E) rendered on entire height of automation editor looks strange. Timing and lenght of notes are right.

I added a minimum height for scaling, so that notes that are less than 20 key apart are rendered as if they were 20 key apart. This means that close together notes should be rendered correctly. Here is your example:

[qnebra]

This looks much better

[IanCaio]

Useful feature! I didn't manage to test it, but code-wise it looks good as far as I could tell. Just left a couple change suggestions.

[IanCaio]

Haven't had the opportunity to test it, but the code LGTM. Approving it with that note so others can test it thoroughly

[qnebra]

Done some testing, doesn't encounter bugs in "normal use". Notes intervals properly rendered, notes lenghts properly rendered, melody shape is rendered as it should be, clear button works properly. Also update feature is really nice, I think 'pianoroll ghost notes' should had it added in another PR.

[superpaik]

It works ok, both with regular and time-zero notes.

Some thoughts after testing:

• do we really need separate ghost notes for piano roll and automation? Personally, I think that just one would be enough.
• we have also per-note detuning automation that works in a different way than "normal" automation. It applies to the note you click, and time 0 is the start of the note, so seen other notes on the editor may be confusing to the user (specially if the note you are detuning is in the ghost notes). I suggest that in that case we don't show ghost notes.

[qnebra]

I think it should be separated, mainly because both had different use cases.
Pianoroll - creating chords for melody or vice versa, creating variations, syncing drums
Automation - precise tweaking of parameters, create 'feeling' of live perfomance

Second point, I didn't think about it. What if when user enter detuning mode, active clip become ghost notes? And on exiting detuning mode previous automation ghost notes are restored?

[IanCaio]

I'm not sure but I think in the code the detuning simply opens the automation editor with an automation clip that corresponds to the detuning, so there's nothing really signaling that we have entered "detuning mode". There's actually one piece of code that checks whether we are editing detuning by checking if the clip has a track (which is actually kinda wrong, cause there could be other inline automations that are not detuning ig):

	// Note detuning?
	if( m_clip && !m_clip->getTrack() )
	{
		getGUI()->pianoRoll()->update();
	}

Because of that, it might require more work to make the ghost notes change to the detuning notes, then change back to the notes it had previously. It's much easier to make it just change automatically to the detuning notes without changing back, so the user would have to set whatever they had before again (it's probably just a couple lines). I'd be fine with that solution for now, and if the OP wants to work further so it changes automatically back then it could be done in a new PR, cause that would require more changes to the automation editor.

[IanCaio]

By the way, making it change automatically to the clip being detuned would probably look like this:

// src/gui/editors/PianoRoll.cpp - void PianoRoll::mousePressEvent(QMouseEvent* me)
// ...
	if( m_editMode == EditMode::Detuning && noteUnderMouse() )
	{
		static QPointer<AutomationClip> detuningClip = nullptr;
		if (detuningClip.data() != nullptr)
		{
			detuningClip->disconnect(this);
		}
		Note* n = noteUnderMouse();
		if (n->detuning() == nullptr)
		{
			n->createDetuning();
		}
		detuningClip = n->detuning()->automationClip();
		connect(detuningClip.data(), SIGNAL(dataChanged()), this, SLOT(update()));
		getGUI()->automationEditor()->setGhostMidiClip(m_midiClip);
		getGUI()->automationEditor()->open(detuningClip);
		return;
	}

Disclaimer: Not tested at all :p

[superpaik]

I'm not sure but I think in the code the detuning simply opens the automation editor with an automation clip that corresponds to the detuning, so there's nothing really signaling that we have entered "detuning mode".

Not the best solution, but I think that m_editor->m_clip->name() has "Note detuning" as it is used as part of the automation window title. Maybe following this path.....

[IanCaio]

I'm not sure but I think in the code the detuning simply opens the automation editor with an automation clip that corresponds to the detuning, so there's nothing really signaling that we have entered "detuning mode".

Not the best solution, but I think that m_editor->m_clip->name() has "Note detuning" as it is used as part of the automation window title. Maybe following this path.....

That's true, that's better than !clip->getTrack() at least, could even change that conditional to use that logic (maybe in another PR). As a more definitive solution I thought of having on the editor class either a boolean like bool editingDetune; or a type that says what kind of editing is being done Type editType; (maybe that one is overkill, I'm not sure there would be other cases like this one for different situations). But for now using the clip name is fine by me too.

[DanielKauss]

we have also per-note detuning automation that works in a different way than "normal" automation. It applies to the note you click, and time 0 is the start of the note, so seen other notes on the editor may be confusing to the user (specially if the note you are detuning is in the ghost notes). I suggest that in that case we don't show ghost notes.

I honestly didn't even know that this feature existed until now.

Thanks for all the suggestions, but I ultimately went with a different approach. Instead of using the name to check for detuning, I compare each note that is being detuned with the current ghost clip. If the detuned note is found, This note gets rendered at the start and selected. Here is an example:

The only problem with this approach is if you edit a note that is not in the ghost clip, since it then simply shows the clip. This could be fixed by just setting the ghost clip once you start detuning. The scaling is also off when the clip gets rendered, since the automation covers the full keyboard. Maybe I could also change the scaling if you are detuning.

Let me know if this is a good solution or if something different would be better.

[IanCaio]

we have also per-note detuning automation that works in a different way than "normal" automation. It applies to the note you click, and time 0 is the start of the note, so seen other notes on the editor may be confusing to the user (specially if the note you are detuning is in the ghost notes). I suggest that in that case we don't show ghost notes.

I honestly didn't even know that this feature existed until now.

Thanks for all the suggestions, but I ultimately went with a different approach. Instead of using the name to check for detuning, I compare each note that is being detuned with the current ghost clip. If the detuned note is found, This note gets rendered at the start and selected. Here is an example:

The only problem with this approach is if you edit a note that is not in the ghost clip, since it then simply shows the clip. This could be fixed by just setting the ghost clip once you start detuning. The scaling is also off when the clip gets rendered, since the automation covers the full keyboard. Maybe I could also change the scaling if you are detuning.

Let me know if this is a good solution or if something different would be better.

I had totally forgot that the detuning would work per note, so having the clip set would not be enough cause of the Offset, nice catch. The solution looks good, you could complement it with the one-liner I posted earlier. Then every time someone started detuning a note the clip would be set automatically.

#6940 (comment)

[DanielKauss]

Reviews are finished and I implemented @IanCaio's comment. I think everything is done.

[IanCaio]

Reviews are finished and I implemented @IanCaio's comment. I think everything is done.

I had approved the changes before the detuning was brought up, just sustaining the approval after the last changes. Again, I didn't manage to test it (though I think others have), but code-wise it looks good to me!

[DanielKauss]

I added additional support for sample tracks, as this was also discussed in #4628. They have the exact same functionality as the ghost notes, and I have noticed no performance issues even with long samples.

Could we merge this? It has been finished for a while now with no complaints.

[sakertooth]

Code looks fine. I'm assuming others have tested the feature already?

Edit: Also, could you try resolving the merge conflicts?

[zonkmachine]

Code looks fine. I'm assuming others have tested the feature already?

I'm testing it right now and it works well. It now has some merge conflict so I suggest I test it some more while @DanielKauss resolves the conflict.

[DanielKauss]

Code looks fine. I'm assuming others have tested the feature already?

I have tested it quite a bit, and no one mentioned any issues.

I'm testing it right now and it works well. It now has some merge conflict so I suggest I test it some more while @DanielKauss resolves the conflict.

Fixed the conflict.

[zonkmachine]

Since 6921ce5 the branch wont build with debug flags, -DCMAKE_BUILD_TYPE=Debug

[ 99%] Linking CXX executable ../lmms
/usr/bin/ld: CMakeFiles/lmmsobjs.dir/gui/editors/AutomationEditor.cpp.o: in function `lmms::gui::AutomationEditor::paintEvent(QPaintEvent*)':
/home/zonkmachine/builds/lmms/src/gui/editors/AutomationEditor.cpp:1289: undefined reference to `lmms::gui::AutomationEditor::MAX_SAMPLE_HEIGHT'
collect2: error: ld returned 1 exit status
make[2]: *** [src/CMakeFiles/lmms.dir/build.make:628: lmms] Error 1
make[1]: *** [CMakeFiles/Makefile2:2181: src/CMakeFiles/lmms.dir/all] Error 2
make: *** [Makefile:156: all] Error 2

[DanielKauss]

Since 6921ce5 the branch wont build with debug flags, -DCMAKE_BUILD_TYPE=Debug

I have absolutely 0 idea what caused this. If you put a print before the error line with printf("%i", MAX_SAMPLE_HEIGHT) it doesn't raise an error about MAX_SAMPLE_HEIGHT missing, but in the line below it does? And now that I replaced it with a ternary the error is gone, even though the same variable is used. Maybe I'm missing something, but this feels very strange.
Anyway, should be fixed now.

[zonkmachine]

I have absolutely 0 idea what caused this.

Well that was bizarre. Nice fix. I'm thinking maybe a comment would be a good idea here?

[DomClark]

MAX_SAMPLE_HEIGHT is a static const member of AutomationEditor. When used in a calculation, like in your fix, the compiler replaces it with its value. However, std::min takes its arguments by reference, so a reference to the variable itself must be passed, not just its value. Linking now fails because the linker cannot find the definition of the variable in order to pass its address, since it is only declared but never defined. A better fix would be to declare the variable as constexpr, which implicitly makes it inline and therefore the declaration is also a definition. See the cppreference page on static members for more information.

The reason this succeeds when not building in debug mode is because the compiler inlines std::min, so it no longer needs to pass the variable by reference and can use its value directly.

[DanielKauss]

MAX_SAMPLE_HEIGHT is a static const member of AutomationEditor. When used in a calculation, like in your fix, the compiler replaces it with its value. However, std::min takes its arguments by reference, so a reference to the variable itself must be passed, not just its value. Linking now fails because the linker cannot find the definition of the variable in order to pass its address, since it is only declared but never defined.

Ok, makes sense now that you explained it, but I would have never guessed that. Any reason for why we don't just use constexpr everywhere in the code? Seems like it should be the default for all fixed compile time numbers to avoid these types of errors.

A better fix would be to declare the variable as constexpr, which implicitly makes it inline and therefore the declaration is also a definition. See the cppreference page on static members for more information.

Done

[DomClark]

Ok, makes sense now that you explained it, but I would have never guessed that.

I agree, it's not at all obvious from looking at the code. What clued me in was that it was specifically a linker error, and compilation had succeeded. That meant that the declaration had been found, but the program needed an actual object in the binary for some reason, which did not exist.

Any reason for why we don't just use constexpr everywhere in the code? Seems like it should be the default for all fixed compile time numbers to avoid these types of errors.

You're absolutely right, we should be using constexpr far more than we do. Reasons that we don't include a lot of LMMS being legacy code, predating C++11 when the keyword was introduced, contributors not being aware of it (perhaps they're new to C++, or learnt it a long time ago before more modern features were introduced), and reviewers not considering it worth commenting on (or missing or being unaware of it themselves). I'm definitely guilty of the latter, and making it more widespread throughout the codebase should help avoid cases where people are unaware of it and make it stand out more when it's omitted.

[zonkmachine]

Merge?

[sakertooth]

Merge?

No objections from me 👍.