Implement Note Types #5902
Implement Note Types #5902
Conversation
Starts implementing Note Types. The two available types are RegularNote and StepNote. PianoRoll now paints the color with a different color for StepNotes. Pattern::addStep now sets the type of the note to StepNote. Negative size is still used to signal a step note.
Updates the methods noteAtStep(), addStepNote() and checkType() from Pattern.cpp to account for the note type and not the note length.
PatternView::paintEvent now draws the pattern if the pattern type is BeatPattern and TCOs aren't fixed (Song Editor). Color used is still the BeatPattern color (grey) and the conditional doesn't look very nice and can be improved. Pattern::beatPatternLength was also updated so it accounts for the note type not note length. Review this method, as it looks a bit weird (particularly the second conditional).
Now, instead of TimePos returning 0 for negative lengths, we create a NotePlayHandle with 0 frames when the note type is StepNote on InstrumentTrack::play.
Improves a conditional inside PatternView::paintEvent by reversing the order in which they are executed.
IanCaio
added
the
after-refactor
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Windows
Linux
macOS🤖{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://13552-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.109%2Bgfcd5797af-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13552?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://13553-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.109%2Bgfcd5797af-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13553?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/pjgko0npbh7265ay/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38753969"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/xlydwesm96ll49ho/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38753969"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://13555-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.109%2Bgfcd5797-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13555?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://13551-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.109%2Bgfcd5797af-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13551?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "f5cc8893f65357a510dd8c6b6a018daa854f4588"} |
|
This also closes #1681. There is an issue brought up somewhere around #1681 (comment) concerning representation of beat notes in tco's that perhaps should have it's own ticket. It's fixed in this PR. |
True, it partially solves it. The notes that are off-beat will still not show up on the BB Step Note editor and neither will it be displayed as a TCO on that case. I think it's better to leave it that way until we can implement allowing step notes to be added from the Piano Roll. I'm about to push the upgrade routine which I totally forgot. Can't load older projects properly without it. |
Adds an upgrade method that converts notes with negative length to StepNotes, so old projects can be loaded properly. Explicitly set the Note::RegularNote value as 0. Make the default "type" value "0", so notes without a type are loaded as RegularNotes.
|
Done. Upgrade method was added, older projects should be loaded properly now. |
|
Note: MIDI export may require some additional changes. |
|
I tested this PR and it works as intended. The PR #5502 should be closed in favor of this one.
Expected Result:
Actual Result:
In the linked PR, this is handled with the |
True, it might not though because it uses
You're right. I'm not sure there's an open issue for that (it seems to be the behavior on master) but we can fix it too. I was looking how you did it, you added an |
I wrote this warning because it depends on the negative length of BB notes. |
I deliberately didn't add it there:
undo/redo, copy/paste works fine, I testet it with #5502. We can open a separate issue for this one. |
Oh, that makes sense, so it would only grow automatically but you'd need to remove the steps manually to shrink it again.
Awesome, I thought there might be some issues because some of them use the XML state and doesn't make calls to I think a separate PR would be better in the testing/bug-hunting aspect, but that's definitely something worth adding. Or I could just add it to this one, I don't mind it neither 😁 |
src/tracks/Pattern.cpp
Outdated
| NoteVector const & noteCollection = m_pat->m_notes; | ||
| if( m_pat->m_patternType == Pattern::MelodyPattern && !noteCollection.empty() ) | ||
|
|
||
| // Beat pattern paint event |
There was a problem hiding this comment.
If we're moving it, let's fix the style before merge? Or just let the eventual clang-format/tidy do it?
There was a problem hiding this comment.
Sure, I ended up not doing it because I moved a big block but didn't change it and wasn't sure how it would show in the diff. Can I leave this style fix for after it's decided whether this will be merged before or after refactor? Because if it's merged after I'm probably going to have to rebase against the fixed-style master anyways
There was a problem hiding this comment.
I'm okay with leaving for clang PR.
- Changes "addStepNote" so "checkType" isn't called twice in a row. - Changes style on a one line conditional.
|
Done! Except for the code style formatting of the big block of code that was moved. I was thinking, another alternative was to change |
Reduces number of lines by using ternary expression.
|
It works fine to me, except in one case. Not strictly related to the functionality but more of UX. I think it would be great to have the possibility to add new Step Notes directly from the pianoroll (now it's only possible through copy-and-paste). Maybe a tool/option to change between type notes would be great. |
I think the issue you experienced is likely this one #2074 . It's probably more noticeable in the piano roll than it is in the BB editor though.
That's the idea! The PR currently only implements the base for allowing that, but following enhancements should allow you to add this type of note directly from the PianoRoll. I even envision other types of notes in the future, like "Arpeggio" notes, so we can move away from the currently buggy Arpeggiator and have a more flexible and visual one. For now, since this will possibly only be merged after the refactor I'd rather just add the base functionality for now and leave those enhancements for later. We'd have to decide how it would work though: Would the Draw mode icon have a dropdown menu where you chose the type of note? Or a context menu for notes on the piano roll? Maybe even both, thought it would require UX changes since the right click is currently used for deleting notes. |
Yes, it seems the same bug. As for your other comment, I agree with you. Let's wait for refactoring. Some ideas as how I see they could work (for the future): I think that, in general, you'd use Regular or Step Notes, but I think it's kind of weird to mix them up (I'm not saying that it shouldn't be addressed, but I see it as an exception). So, if the piano roll is open form inside the BBEditor, I'd set the draw to Step notes by default, and if it's open from the song editor, the Regular Notes should by the default. Maybe, to change the draw mode, just by clicking on the draw icon it could go from one type to the other. Also, the "last note" draw option should have into account the different type notes, so it's easier for the user to change note type. |
|
Reminder to self: lmms/src/gui/editors/PianoRoll.cpp Lines 1591 to 1596 in e880e3c This piece of code on the Out of the scope of this PR but lmms/src/gui/editors/PianoRoll.cpp Lines 1599 to 1600 in e880e3c
|
I've been slowly hacking at all mouse events in PianoRoll. I have a branch locally which has rewritten a lot to use switch's and organizing the code to be a lot more readable. It's still a big WIP though. |
I was actually thinking of asking if you wanted to team up fixing those mouse event methods, maybe even coming up with controls that were more up-to-date to the current new features: One thing I was thinking of was implementing a context-menu, we could move the 4 note tools and the glue tool to that context menu, opening space for many others without filling the window toolbar. It's awesome that you have this WIP going already, just moving to a more organized |
|
Conflicts with master fixed |
- Addresses code review from PhysSong and Messmerd
- Notes were not being draw on the song editor for BeatClips. This commit fixes this.
|
Thanks guys, code reviews were addressed! 😄 Last commit fixes a small conditional issue on the drawing of MidiClips on the song editor (Beat clips weren't having their notes drawn). It's a small fix, if you guys can check if it's good then it's all done. I could have left the conditional as only |
|
@IanCaio It looks like |
- Adds header to use assert() on TimePos.cpp
Weird, for some reason it compiled fine here, but I just pushed a new commit with the header. Let's see if the builds work now! |
plugins/MidiExport/MidiExport.cpp
Outdated
| @@ -113,6 +114,7 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks, | |||
| int base_pitch = 0; | |||
| double base_volume = 1.0; | |||
| int base_time = 0; | |||
| int base_beatLen = 16; | |||
There was a problem hiding this comment.
Looks like there are a few places where the default step note length of 16 is hard-coded in. You should probably create a constexpr constant for it and use that instead.
There was a problem hiding this comment.
I just followed the example of the other variables, that value is more like a default in case there's no stored value on the XML. Should I change it to be constexpr int base_beatLen = 16; here and in the other method?
There was a problem hiding this comment.
No, I meant if you want the default step note length to be a 1/16th note, you shouldn't have "16" hard-coded in 3 or 4 different places. You should probably use a static constexpr member in the Note class called DefaultStepNoteLength or something like that:
// Inside Note class in Note.h
static constexpr int DefaultStepNoteLength = 16; // 16th noteThen replace every "16" which you're using to represent the default length of step notes with Note::DefaultStepNoteLength.
The point of this is to avoid magic numbers and have the default step note length defined in a single place so it's easy to change later if we ever decide to do that.
There was a problem hiding this comment.
Oh, got it! I wrote the changes here, I'm just double checking because I think base_beatLen is actually not related to the default step note length of 1/16th (the one that shows on the piano roll):
Updates MidiExport to use Note Types
- Now MidiExport is updated to use note types instead of relying
on negative length notes.- For that change it was necessary to find a way of letting
MidiExport know how long step notes should be. The solution found was to
add an attribute to the Instrument XML called "beatlen", which would
hold the number of frames of the instrument's beat. That would be
converted to ticks, so we could calculate how long the MIDI notes would
have to be to play the whole step note. If the attribute was not found,
the default value of 16 ticks would be used as a length of step notes,
as a fallback.
I think I'd need another constexpr for the beatLen fallback value. Should I have it on "InstrumentTrack.h"?
Fixes style on some lines Co-authored-by: Dalton Messmer <[email protected]>
- Some changes were reverted on MidiExport and InstrumentTrack. We were storing the beat length on the XML of Instrument Tracks, but in reality the beat length is a per note attribute, and some instruments could run into a segmentation fault when calling beat length without a NotePlayHandle (i.e.: AFP). Because of that I reverted this change, so the beat length is not stored on the XML anymore, and instead we have a magic number on the MidiExport class that holds a default beat length which is actually an upper limit for the MIDI notes of step notes. In the future we can improve this by finding a way to store the beat length on the note class to use it instead. The MidiExport logic is not worsened at all because previously the beat length wasn't even considered during export (it was actually improved making the exported notes extend until the next one instead of cutting shorter).
|
I reverted one of the changes in MidiExport. Turns out So now a magic number is being used as a default beat length, that in practice just represents the upper limit of the length of a step note on the MIDI in ticks. I left a TODO comment so this is clear, and in no way this worsens the MidiExport logic, since the previous code didn't care about the beat length at all. It's probably even improved because before the step notes were cut short since the negative length of step notes was used as a limit instead. That should be the last change hopefully. @DomClark do you still wish to review this? And the other reviewers can check whether everything they suggested was covered? |
|
Does anyone still wishes to review this PR? |
There was a problem hiding this comment.
Tested and my projects work as they should. Two issues are solved by this and I can confirm the issues and the fixes.
There's a qWarning left on TimePos::frames that is supposed to be removed. Keeping it just to check if there's any code left creating negative length notes. Didn't rush to remove it since I'm not even sure this will be reviewed before the refactor.
This is in the top post. It looks like this has been fixed, right?
|
@IanCaio I introduced a new merge conflict in the latest merge. Just a final 'touch up' and then merge, I say. |
|
Just fixed the conflicts and the order of included files |
messmerd
changed the title
This PR moves away from using negative length for notes that are supposed to be played for the length of the sample. Instead, we now use Note Types, which describe the type of note that is being played (for now, either
RegularNoteorStepNote). The length of a step note is now 1/16th by default, but it doesn't matter if you resize it becauseInstrumentTrack::playwill create a NotePlayHandle with 0 frames if the type of the note isStepNote.The StepNotes are drawn now as red notes on the PianoRoll. BeatPattern TCOs are drawn in the Song Editor with the assigned BBPatternBackground color (which is grey on default theme).
On the image, TripleOsc with a MelodyPattern and AFP with a BeatPattern and a MelodyPattern that has both step notes and regular notes (opened on the PianoRoll):
There's a
qWarningleft onTimePos::framesthat is supposed to be removed. Keeping it just to check if there's any code left creating negative length notes. Didn't rush to remove it since I'm not even sure this will be reviewed before the refactor.Fixes #5339
Fixes #5340