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.

Sign up for GitHub

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

Jump to bottom

Implement Note Types #5902

Merged
merged 27 commits into from
Nov 18, 2023
Merged

Implement Note Types #5902

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
b205222
Initial Commit
IanCaio Feb 3, 2021
cfb533a
Update Pattern.cpp to account for the Note::Type
IanCaio Feb 3, 2021
a5cf05d
Update PatternView::paintEvent to draw step notes
IanCaio Feb 3, 2021
df88140
Implements StepNotes setting a NPH with 0 frames
IanCaio Feb 3, 2021
7a5ad82
Improves PatternView::paintEvent conditional
IanCaio Feb 3, 2021
725169a
Adds upgrade method for backwards compatibility
IanCaio Feb 3, 2021
faa5209
Addresses Veratil's review
IanCaio Feb 11, 2021
9decf11
Uses ternary expression on statement
IanCaio Feb 11, 2021
3576300
Merge branch 'master' into feature/BBNotes
IanCaio Mar 5, 2021
f5cc889
Merge branch 'master' into feature/BBNotes
IanCaio Apr 18, 2021
1df46a5
Merge branch 'master' into feature/BBNotes
IanCaio Jul 7, 2023
db0d0b7
Addresses PR review (sakertooth)
IanCaio Jul 9, 2023
6b3c75e
Finished changes from review (sakertooth)
IanCaio Jul 9, 2023
4b69615
Uses std::find_if to save codelines
IanCaio Jul 10, 2023
89a62f6
Addresses review from sakertooth
IanCaio Jul 17, 2023
b3511c2
Addresses DomClark's review
IanCaio Jul 23, 2023
dbefa9c
Updates MidiExport to use Note Types
IanCaio Aug 21, 2023
463050a
Merge branch 'master' into feature/BBNotes
IanCaio Aug 28, 2023
0054ece
Fixes ambiguity on enum usage
IanCaio Aug 28, 2023
fe42e97
Merge branch 'master' into feature/BBNotes
IanCaio Oct 17, 2023
38dd8ec
Addresses new code reviews
IanCaio Oct 17, 2023
31a2bd3
Fixes note drawing on Song Editor
IanCaio Oct 17, 2023
43018d7
Adds cassert header to TimePos.cpp
IanCaio Oct 18, 2023
389a245
Apply suggestions from code review
IanCaio Oct 26, 2023
dcb41af
Reverts some changes on MidiExport
IanCaio Nov 3, 2023
2ecadb0
Merge remote-tracking branch 'upstream/master' into feature/BBNotes
IanCaio Nov 18, 2023
67b0b24
Fix the order of included files
IanCaio Nov 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions data/themes/classic/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ lmms--gui--PianoRoll {
qproperty-backgroundShade: rgba( 255, 255, 255, 10 );
qproperty-noteModeColor: rgb( 255, 255, 255 );
qproperty-noteColor: rgb( 119, 199, 216 );
qproperty-stepNoteColor: #9b1313;
qproperty-noteTextColor: rgb( 255, 255, 255 );
qproperty-noteOpacity: 128;
qproperty-noteBorders: true; /* boolean property, set false to have borderless notes */
Expand Down
1 change: 1 addition & 0 deletions data/themes/default/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ lmms--gui--PianoRoll {
qproperty-backgroundShade: rgba(255, 255, 255, 10);
qproperty-noteModeColor: #0bd556;
qproperty-noteColor: #0bd556;
qproperty-stepNoteColor: #9b1313;
qproperty-noteTextColor: #ffffff;
qproperty-noteOpacity: 165;
qproperty-noteBorders: false; /* boolean property, set false to have borderless notes */
Expand Down
1 change: 1 addition & 0 deletions include/DataFile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ class LMMS_EXPORT DataFile : public QDomDocument
void upgrade_bbTcoRename();
void upgrade_sampleAndHold();
void upgrade_midiCCIndexing();
void upgrade_noteTypes();
IanCaio marked this conversation as resolved.
Show resolved Hide resolved

// List of all upgrade methods
static const std::vector<UpgradeMethod> UPGRADE_METHODS;
Expand Down
15 changes: 15 additions & 0 deletions include/Note.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,19 @@ class LMMS_EXPORT Note : public SerializingObject
Note( const Note & note );
~Note() override;

// Note types
enum class Type
{
Regular = 0,
Step
};
messmerd marked this conversation as resolved.
Show resolved Hide resolved

Type type() const
{
return m_type;
}
inline void setType(Type t) { m_type = t; }
IanCaio marked this conversation as resolved.
Show resolved Hide resolved

// used by GUI
inline void setSelected( const bool selected ) { m_selected = selected; }
inline void setOldKey( const int oldKey ) { m_oldKey = oldKey; }
Expand Down Expand Up @@ -253,6 +266,8 @@ class LMMS_EXPORT Note : public SerializingObject
TimePos m_length;
TimePos m_pos;
DetuningHelper * m_detuning;

Type m_type = Type::Regular;
};

using NoteVector = std::vector<Note*>;
Expand Down
2 changes: 2 additions & 0 deletions include/PianoRoll.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class PianoRoll : public QWidget
Q_PROPERTY(QColor lineColor MEMBER m_lineColor)
Q_PROPERTY(QColor noteModeColor MEMBER m_noteModeColor)
Q_PROPERTY(QColor noteColor MEMBER m_noteColor)
Q_PROPERTY(QColor stepNoteColor MEMBER m_stepNoteColor)
Q_PROPERTY(QColor ghostNoteColor MEMBER m_ghostNoteColor)
Q_PROPERTY(QColor noteTextColor MEMBER m_noteTextColor)
Q_PROPERTY(QColor ghostNoteTextColor MEMBER m_ghostNoteTextColor)
Expand Down Expand Up @@ -466,6 +467,7 @@ protected slots:
QColor m_lineColor;
QColor m_noteModeColor;
QColor m_noteColor;
QColor m_stepNoteColor;
QColor m_noteTextColor;
QColor m_ghostNoteColor;
QColor m_ghostNoteTextColor;
Expand Down
33 changes: 28 additions & 5 deletions plugins/MidiExport/MidiExport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

#include "MidiExport.h"

#include "Engine.h"
#include "TrackContainer.h"
#include "DataFile.h"
#include "InstrumentTrack.h"
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 note

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


MidiNoteVector midiClip;

Expand All @@ -128,6 +130,17 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
base_pitch += masterPitch;
}
base_volume = LocaleHelper::toDouble(it.attribute("volume", "100"))/100.0;
// From the PR 5902 forward (Note Types), the instrument XML includes the beat length
// in frames. We try to fetch it and convert to the length in ticks. If there's no beat
// length in XML, the default beat length of 16 ticks will be used.
QDomNode iNode = it.elementsByTagName("instrument").at(0);
if(!iNode.isNull()){
QDomElement i = iNode.toElement();
if(i.hasAttribute("beatlen"))
IanCaio marked this conversation as resolved.
Show resolved Hide resolved
{
base_beatLen = i.attribute("beatlen", "0").toInt() / Engine::framesPerTick();
}
}
}

if (n.nodeName() == "midiclip")
Expand All @@ -137,7 +150,7 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
}

}
processPatternNotes(midiClip, INT_MAX);
processPatternNotes(midiClip, base_beatLen, INT_MAX);
writeMidiClipToTrack(mtrack, midiClip);
size = mtrack.writeToBuffer(buffer.data());
midiout.writeRawData((char *)buffer.data(), size);
Expand Down Expand Up @@ -188,6 +201,7 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,

int base_pitch = 0;
double base_volume = 1.0;
int base_beatLen = 16;

// for each pattern in the pattern editor
for (QDomNode n = element.firstChild(); !n.isNull(); n = n.nextSibling())
Expand All @@ -201,6 +215,13 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
base_pitch += masterPitch;
}
base_volume = LocaleHelper::toDouble(it.attribute("volume", "100")) / 100.0;
QDomNode iNode = it.elementsByTagName("instrument").at(0);
if(!iNode.isNull()){
QDomElement i = iNode.toElement();
if(i.hasAttribute("beatlen")){
IanCaio marked this conversation as resolved.
Show resolved Hide resolved
base_beatLen = i.attribute("beatlen", "0").toInt()/Engine::framesPerTick();
}
}
}

if (n.nodeName() == "midiclip")
Expand Down Expand Up @@ -247,7 +268,7 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
st.pop_back();
}

processPatternNotes(nv, pos);
processPatternNotes(nv, base_beatLen, pos);
writeMidiClipToTrack(mtrack, nv);

// next pattern track
Expand Down Expand Up @@ -279,6 +300,7 @@ void MidiExport::writeMidiClip(MidiNoteVector &midiClip, const QDomNode& n,
mnote.volume = qMin(qRound(base_volume * LocaleHelper::toDouble(note.attribute("vol", "100")) * (127.0 / 200.0)), 127);
mnote.time = base_time + note.attribute("pos", "0").toInt();
mnote.duration = note.attribute("len", "0").toInt();
mnote.type = static_cast<Note::Type>(note.attribute("type", "0").toInt());
midiClip.push_back(mnote);
}
}
Expand Down Expand Up @@ -311,14 +333,15 @@ void MidiExport::writePatternClip(MidiNoteVector& src, MidiNoteVector& dst,
note.pitch = srcNote.pitch;
note.time = base + time;
note.volume = srcNote.volume;
note.type = srcNote.type;
dst.push_back(note);
}
}
}



void MidiExport::processPatternNotes(MidiNoteVector& nv, int cutPos)
void MidiExport::processPatternNotes(MidiNoteVector& nv, int beatLen, int cutPos)
{
std::sort(nv.begin(), nv.end());
int cur = INT_MAX, next = INT_MAX;
Expand All @@ -329,9 +352,9 @@ void MidiExport::processPatternNotes(MidiNoteVector& nv, int cutPos)
next = cur;
cur = it->time;
}
if (it->duration < 0)
if (it->type == Note::Type::Step)
{
it->duration = qMin(qMin(-it->duration, next - cur), cutPos - it->time);
it->duration = qMin(qMin(beatLen, next - cur), cutPos - it->time);
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion plugins/MidiExport/MidiExport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

#include "ExportFilter.h"
#include "MidiFile.hpp"
#include "Note.h"

class QDomNode;

Expand All @@ -46,6 +47,7 @@ struct MidiNote
uint8_t pitch;
int duration;
uint8_t volume;
Note::Type type;

inline bool operator<(const MidiNote &b) const
{
Expand Down Expand Up @@ -78,7 +80,7 @@ class MidiExport: public ExportFilter
void writeMidiClipToTrack(MTrack &mtrack, MidiNoteVector &nv);
void writePatternClip(MidiNoteVector &src, MidiNoteVector &dst,
int len, int base, int start, int end);
void processPatternNotes(MidiNoteVector &nv, int cutPos);
void processPatternNotes(MidiNoteVector &nv, int beatLen, int cutPos);

void error();

Expand Down
22 changes: 21 additions & 1 deletion src/core/DataFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#include "embed.h"
#include "GuiApplication.h"
#include "LocaleHelper.h"
#include "Note.h"
#include "PluginFactory.h"
#include "ProjectVersion.h"
#include "SongEditor.h"
Expand Down Expand Up @@ -79,7 +80,8 @@ const std::vector<DataFile::UpgradeMethod> DataFile::UPGRADE_METHODS = {
&DataFile::upgrade_automationNodes , &DataFile::upgrade_extendedNoteRange,
&DataFile::upgrade_defaultTripleOscillatorHQ,
&DataFile::upgrade_mixerRename , &DataFile::upgrade_bbTcoRename,
&DataFile::upgrade_sampleAndHold , &DataFile::upgrade_midiCCIndexing
&DataFile::upgrade_sampleAndHold , &DataFile::upgrade_midiCCIndexing,
&DataFile::upgrade_noteTypes
};

// Vector of all versions that have upgrade routines.
Expand Down Expand Up @@ -1662,6 +1664,24 @@ void DataFile::upgrade_automationNodes()
}
}

// Convert the negative length notes to StepNotes
void DataFile::upgrade_noteTypes()
{
const auto notes = elementsByTagName("note");

for (int i = 0; i < notes.size(); ++i)
{
auto note = notes.item(i).toElement();

const auto noteSize = note.attribute("len").toInt();
if (noteSize < 0)
{
note.setAttribute("len", DefaultTicksPerBar / 16);
note.setAttribute("type", static_cast<int>(Note::Type::Step));
}
}
}


/** \brief Note range has been extended to match MIDI specification
*
Expand Down
6 changes: 5 additions & 1 deletion src/core/Note.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ Note::Note( const Note & note ) :
m_panning( note.m_panning ),
m_length( note.m_length ),
m_pos( note.m_pos ),
m_detuning( nullptr )
m_detuning(nullptr),
IanCaio marked this conversation as resolved.
Show resolved Hide resolved
m_type(note.m_type)
{
if( note.m_detuning )
{
Expand Down Expand Up @@ -179,6 +180,7 @@ void Note::saveSettings( QDomDocument & doc, QDomElement & parent )
parent.setAttribute( "pan", m_panning );
parent.setAttribute( "len", m_length );
parent.setAttribute( "pos", m_pos );
parent.setAttribute("type", static_cast<int>(m_type));

if( m_detuning && m_length )
{
Expand All @@ -197,6 +199,8 @@ void Note::loadSettings( const QDomElement & _this )
m_panning = _this.attribute( "pan" ).toInt();
m_length = _this.attribute( "len" ).toInt();
m_pos = _this.attribute( "pos" ).toInt();
// Default m_type value is 0, which corresponds to RegularNote
m_type = static_cast<Type>(_this.attribute("type", "0").toInt());
IanCaio marked this conversation as resolved.
Show resolved Hide resolved

if( _this.hasChildNodes() )
{
Expand Down
2 changes: 1 addition & 1 deletion src/core/NotePlayHandle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ NotePlayHandle::NotePlayHandle( InstrumentTrack* instrumentTrack,
NotePlayHandle *parent,
int midiEventChannel,
Origin origin ) :
PlayHandle( Type::NotePlayHandle, _offset ),
PlayHandle( PlayHandle::Type::NotePlayHandle, _offset ),
Note( n.length(), n.pos(), n.key(), n.getVolume(), n.getPanning(), n.detuning() ),
m_pluginData( nullptr ),
m_instrumentTrack( instrumentTrack ),
Expand Down
8 changes: 2 additions & 6 deletions src/core/TimePos.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,7 @@ tick_t TimePos::getTickWithinBeat( const TimeSig &sig ) const

f_cnt_t TimePos::frames( const float framesPerTick ) const
{
if( m_ticks >= 0 )
{
return static_cast<f_cnt_t>( m_ticks * framesPerTick );
}
return 0;
return static_cast<f_cnt_t>(m_ticks * framesPerTick);
IanCaio marked this conversation as resolved.
Show resolved Hide resolved
}

double TimePos::getTimeInMilliseconds( bpm_t beatsPerMinute ) const
Expand Down Expand Up @@ -221,4 +217,4 @@ double TimePos::ticksToMilliseconds(double ticks, bpm_t beatsPerMinute)
}


} // namespace lmms
} // namespace lmms
Loading
Loading