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

Add ability for region playlist crop and append to include tempo mark… #1562

Closed
wants to merge 2 commits into from
Closed

Add ability for region playlist crop and append to include tempo mark… #1562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fd1c57
Add ability for region playlist crop and append to include tempo mark…
PSchwilden Oct 1, 2021
f8a288d
Add extra tempo marker parameter (beatBase) and change default values…
PSchwilden Oct 17, 2022
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
225 changes: 225 additions & 0 deletions SnM/SnM_Marker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,3 +345,228 @@ void InsertMarker(COMMAND_T* _ct)
UpdateTimeline();
Undo_OnStateChangeEx2(NULL, SWS_CMD_SHORTNAME(_ct), UNDO_STATE_MISCCFG, -1);
}

TempoMarker::TempoMarker(double _dTimePos, int _measurePos, double _dBeatPos, double _dBpm, int _timesig_num, int _timesig_denom, bool _bLinearTempo, RawTempoMarkerData _rawData)
{
m_dTimePos = _dTimePos;
m_measurePos = _measurePos;
m_dBeatPos = _dBeatPos;
m_dBpm = _dBpm;
m_timesig_num = _timesig_num;
m_timesig_denom = _timesig_denom;
m_bLinearTempo = _bLinearTempo;
m_rawData = _rawData;
}

bool GetAllTempoMarkers(WDL_PtrList<void>* _tempoMarkers, ReaProject* _proj)
Copy link
Member

Choose a reason for hiding this comment

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

Why void*? WDL_PtrList<TempoMarker> would be clearer and safer by not requiring a cast on access.

{
if (_tempoMarkers)
Copy link
Member

Choose a reason for hiding this comment

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

The caller code would be completely broken if it passed a null tempoMarkers. It should crash instead of silently failing.

In this case though it would better to just build a local list and return it:

WDL_PtrList<TempoMarker> GetAllTempoMarkers(ReaProject* _proj);

At the very least, an indentation level could be saved with

if (!_tempoMarkers)
  return;

{
_tempoMarkers->Empty();
double dTimePosOut;
int measurePosOut;
double dBeatPosOut;
double dBpmOut;
int timesig_numOut;
int timesig_denomOut;
bool bLinearTempoOut;
Comment on lines +366 to +372
Copy link
Member

@cfillion cfillion Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

These are pretty far from their use and in a wider scope. And since these are most of TempoMarker members duplicated, it could just be a TempoMarker instead.


map<int, RawTempoMarkerData> tempoDataMap;
Copy link
Member

@cfillion cfillion Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A map is overkill for contiguous 0-based integer keys. A simple vector would suffice (and be more efficient, especially if pre-reserving space for CountTempoTimeSigMarkers items).


MediaTrack* masterTrack = GetMasterTrack(_proj);
TrackEnvelope* tempoEnvEx = GetTrackEnvelope(masterTrack, 0);
Copy link
Member

@cfillion cfillion Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Envelope 0 is not always the tempo envelope if there are other envelopes on the master track.

GetTrackEnvelopeByName(masterTrack, __LOCALIZE("Tempo map", "env"))

string tempoStateStr = envelope::GetEnvelopeStateChunkBig(tempoEnvEx);
char* envState = &tempoStateStr[0];
int index = 0;

char* token = strtok(envState, "\n");

while (token != NULL)
{
RawTempoMarkerData newTempoMarker = RawTempoMarkerData();
if (sscanf(token, "PT %lf %lf %d %u %d %d %d %s %u %u %d", &newTempoMarker.position, &newTempoMarker.bpm, &newTempoMarker.linearBool, &newTempoMarker.beatDivision, &newTempoMarker.selectionBool, &newTempoMarker.settingsBitmask, &newTempoMarker.bezierTension, newTempoMarker.quantizationSettings, &newTempoMarker.metronomePatternL32, &newTempoMarker.metronomePatternH32, &newTempoMarker.beatBase) >= 2)
Copy link
Member

Choose a reason for hiding this comment

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

quantizationSettings[10] can overflow here!

Better to use LineParser (the official parser from Cockos) for reading chunk lines.

{
tempoDataMap[index] = newTempoMarker;
index++;
}
token = strtok(NULL, "\n");
}

for (int i = 0; i <= CountTempoTimeSigMarkers(_proj); i++)
Copy link
Member

Choose a reason for hiding this comment

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

Off-by-one error: GetTempoTimeSigMarker(count) does not exist.

{
if (GetTempoTimeSigMarker(_proj, i, &dTimePosOut, &measurePosOut, &dBeatPosOut, &dBpmOut, &timesig_numOut, &timesig_denomOut, &bLinearTempoOut))
{
if (tempoDataMap.find(i) != tempoDataMap.end())
{
TempoMarker* tempoMarker = new TempoMarker(dTimePosOut, measurePosOut, dBeatPosOut, dBpmOut, timesig_numOut, timesig_denomOut, bLinearTempoOut, tempoDataMap.find(i)->second);
Copy link
Member

Choose a reason for hiding this comment

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

These TempoMarker are leaking. Could use a WDL_PtrList_DeleteOnDestroy, but in any case there's no point in allocating this small data structure on the heap and making a list of their pointers.

This TempoMarker could be on the stack then copied into a std::vector<TempoMap> (whose data is stored in the heap anyway).

_tempoMarkers->Add(tempoMarker);
}
}
}
}
return (_tempoMarkers && _tempoMarkers->GetSize());
}

bool GetTempoMarkersInInterval(WDL_PtrList<void>* _tempoMarkers, ReaProject* _proj, double _pos1, double _pos2)
Copy link
Member

Choose a reason for hiding this comment

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

Lots of duplicate code here. Should be extracted & reused.

{
if (_tempoMarkers)
{
_tempoMarkers->Empty();
double dTimePosOut;
int measurePosOut;
double dBeatPosOut;
double dBpmOut;
int timesig_numOut;
int timesig_denomOut;
bool bLinearTempoOut;

map<int, RawTempoMarkerData> tempoDataMap;

MediaTrack* masterTrack = GetMasterTrack(_proj);
TrackEnvelope* tempoEnvEx = GetTrackEnvelope(masterTrack, 0);
string tempoStateStr = envelope::GetEnvelopeStateChunkBig(tempoEnvEx);
char* envState = &tempoStateStr[0];
int index = 0;

char* token = strtok(envState, "\n");

while (token != NULL)
{
RawTempoMarkerData newTempoMarker = RawTempoMarkerData();
if (sscanf(token, "PT %lf %lf %d %u %d %d %d %s %u %u %d", &newTempoMarker.position, &newTempoMarker.bpm, &newTempoMarker.linearBool, &newTempoMarker.beatDivision, &newTempoMarker.selectionBool, &newTempoMarker.settingsBitmask, &newTempoMarker.bezierTension, newTempoMarker.quantizationSettings, &newTempoMarker.metronomePatternL32, &newTempoMarker.metronomePatternH32, &newTempoMarker.beatBase) >= 2)
{
tempoDataMap[index] = newTempoMarker;
index++;
}
token = strtok(NULL, "\n");
}

for (int i = 0; i <= CountTempoTimeSigMarkers(_proj); i++)
{
if (GetTempoTimeSigMarker(_proj, i, &dTimePosOut, &measurePosOut, &dBeatPosOut, &dBpmOut, &timesig_numOut, &timesig_denomOut, &bLinearTempoOut))
{
if (tempoDataMap.find(i) != tempoDataMap.end())
{
TempoMarker* tempoMarker = new TempoMarker(dTimePosOut, measurePosOut, dBeatPosOut, dBpmOut, timesig_numOut, timesig_denomOut, bLinearTempoOut, tempoDataMap.find(i)->second);
if (IsTempoMarkerInInterval(tempoMarker, _pos1, _pos2))
Copy link
Member

Choose a reason for hiding this comment

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

tempoMarker leaks if false. (Well, it's never freed anyway even if true... as mentioned above)

else
  delete tempoMarker;

_tempoMarkers->Add(tempoMarker);
}
}
}
}
return (_tempoMarkers && _tempoMarkers->GetSize());
}

bool IsTempoMarkerInInterval(TempoMarker* _tempoMarker, double _pos1, double _pos2)
{
if (_tempoMarker)
{
double pos = _tempoMarker->GetTimePosition();
if (pos >= _pos1 && pos <= _pos2) // is within?
return true;
}
return false;
}
Comment on lines +460 to +469
Copy link
Member

Choose a reason for hiding this comment

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

Since it being nullable has no purpose (would be a bug anyway), it could be a reference.

Suggested change
bool IsTempoMarkerInInterval(TempoMarker* _tempoMarker, double _pos1, double _pos2)
{
if (_tempoMarker)
{
double pos = _tempoMarker->GetTimePosition();
if (pos >= _pos1 && pos <= _pos2) // is within?
return true;
}
return false;
}
bool IsTempoMarkerInInterval(const TempoMarker &tempoMarker, const double rangeStart, const double rangeEnd)
{
const double pos = tempoMarker.GetTimePosition();
return pos >= rangeStart && pos <= rangeEnd;
}


bool DuplicateTempoMarkers(WDL_PtrList<void>* _tempoMarkers, ReaProject* _proj, const char* _undoTitle, double _nudgePos)
{
bool updated = false;

if (_undoTitle)
Undo_BeginBlock2(NULL);

if (_tempoMarkers)
{
std::map<double, RawTempoMarkerData> newMarkersPosition;

WDL_PtrList<void> oldTempoMarkers = *_tempoMarkers;
Copy link
Member

Choose a reason for hiding this comment

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

Why copy the list?


for (int i = 0; i < oldTempoMarkers.GetSize(); i++)
{
if (TempoMarker* oldTempoMarker = (TempoMarker*)oldTempoMarkers.Get(i))
{
double oldPos = oldTempoMarker->GetTimePosition();
double newPos = oldPos + _nudgePos;
SetTempoTimeSigMarker(_proj, -1, newPos,-1,-1, oldTempoMarker->GetBPM(), oldTempoMarker->GetTimeSignatureNumerator(), oldTempoMarker->GetTimeSignatureDenominator(), oldTempoMarker->IsLinearTempo());
Copy link
Member

Choose a reason for hiding this comment

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

Since this function overwrites the entire envelope state chunk later on, couldn't it just insert the new markers into it and skip everything else?


newMarkersPosition[newPos]= oldTempoMarker->GetRawMarkerData();
}
}

double dTimePosOut;
int measurePosOut;
double dBeatPosOut;
double dBpmOut;
int timesig_numOut;
int timesig_denomOut;
bool bLinearTempoOut;
std::map<int, RawTempoMarkerData> newMarkersId;
for (int i = 0; i < CountTempoTimeSigMarkers(_proj); i++)
{
if (GetTempoTimeSigMarker(_proj, i, &dTimePosOut, &measurePosOut, &dBeatPosOut, &dBpmOut, &timesig_numOut, &timesig_denomOut, &bLinearTempoOut))
{
if (newMarkersPosition.find(dTimePosOut) != newMarkersPosition.end())
{
newMarkersId[i] = newMarkersPosition.find(dTimePosOut)->second;
}
}
}

MediaTrack* masterTrack = GetMasterTrack(_proj);
TrackEnvelope* tempoEnvEx = GetTrackEnvelope(masterTrack, 0);
string tempoStateStr = envelope::GetEnvelopeStateChunkBig(tempoEnvEx);
char* envState = &tempoStateStr[0];
string newState = "";
int index = 0;
double position;
int settingsBitmask;
char* token = strtok(envState, "\n");

while (token != NULL)
{
int foundValues = sscanf(token, "PT %lf %*lf %*d %*u %*d %d %*d %*s %*u %*u %*d", &position, &settingsBitmask);
if (foundValues >= 1)
{
std::map<int, RawTempoMarkerData>::iterator findResult = newMarkersId.find(index);

if (findResult != newMarkersId.end())
{
string newToken = token;
char appendString[2048];
if (foundValues == 1)
{
sprintf(appendString, " %u %d", findResult->second.beatDivision, findResult->second.selectionBool);
newToken.append(appendString);
}
else
{
newToken = newToken.substr(0, newToken.size() - 2);
}

sprintf(appendString, " %d %d %s %u %u %d", findResult->second.settingsBitmask, findResult->second.bezierTension, findResult->second.quantizationSettings, findResult->second.metronomePatternL32, findResult->second.metronomePatternH32, findResult->second.beatBase);
newToken.append(appendString);
newState.append(newToken);
newState.append("\n");
token = strtok(NULL, "\n");
index++;
continue;
}

index++;
}
newState.append(token);
newState.append("\n");
token = strtok(NULL, "\n");
}

SetEnvelopeStateChunk(tempoEnvEx, newState.c_str(), false);

}

if (_undoTitle) {
UpdateArrange();
Undo_EndBlock2(NULL, _undoTitle, UNDO_STATE_ALL);
}
return updated;
}

57 changes: 57 additions & 0 deletions SnM/SnM_Marker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,61 @@ class MarkerRegion : public MarkerItem {
int m_id;
};

//Handle tempo markers for region playlist
struct RawTempoMarkerData
{
double position = 0;
double bpm = 0;
int linearBool = 0;
unsigned int beatDivision = 0; //High 16 bits are time signature bottom, low 16 bits are time signature top
int selectionBool = 0; //Is this point selected
int settingsBitmask = 0;
int bezierTension = 0; //Not used for tempo markers
char quantizationSettings[10]; //Not sure how this works but I guess that if you quantize the tempo marker it would change this.
unsigned int metronomePatternL32 = 0;
unsigned int metronomePatternH32 = 0;
int beatBase = 0;
};

class TempoMarker
{
public:
TempoMarker(double _dTimePos, int _measurePos, double _dBeatPos, double _dBpm, int _timesig_num, int _timesig_denom, bool _bLinearTempo, RawTempoMarkerData _rawData);
~TempoMarker() {}

double GetTimePosition() { return m_dTimePos; }
void SetTimePosition(double dTimePos) { m_dTimePos = dTimePos; }
int GetMeasurePosition() { return m_measurePos; }
void SetMeasurePosition(int measurePos) { m_measurePos = measurePos; }
double GetBeatPosition() { return m_dBeatPos; }
void SetBeatPosition(double dBeatPos) { m_dBeatPos = dBeatPos; }
double GetBPM() { return m_dBpm; }
void SetBPM(double dBpm) { m_dBpm = dBpm; }
int GetTimeSignatureNumerator() { return m_timesig_num; }
void SetTimeSignatureNumerator(int timesig_num) { m_timesig_num = timesig_num; }
int GetTimeSignatureDenominator() { return m_timesig_denom; }
void SetTimeSignatureDenominator(int timesig_denom) { m_timesig_denom = timesig_denom; }
bool IsLinearTempo() { return m_bLinearTempo; }
void SetIsLinearTempo(bool bLinearTempo) { m_bLinearTempo = bLinearTempo; }
RawTempoMarkerData GetRawMarkerData() { return m_rawData; }
void SetRawMarkerData(RawTempoMarkerData rawData) { m_rawData = rawData; };
Comment on lines +96 to +111
Copy link
Member

@cfillion cfillion Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Most of these are unused. Since it's just a simple POD struct, direct member access is OK. (Also protected instead of private implies this class is meant to be inherited.)

struct TempoMarker {
  	double m_dTimePos;
	int m_measurePos;
	double m_dBeatPos, m_dBpm;
	int m_timesig_num, m_timesig_denom;
	bool m_bLinearTempo;
	RawTempoMarkerData m_rawData; // why a separate struct? most of the data overlap
};


protected:
double m_dTimePos;
int m_measurePos;
double m_dBeatPos;
double m_dBpm;
int m_timesig_num;
int m_timesig_denom;
bool m_bLinearTempo;
RawTempoMarkerData m_rawData;

};



bool GetAllTempoMarkers(WDL_PtrList<void>* _tempoMarkers, ReaProject* _proj);
bool GetTempoMarkersInInterval(WDL_PtrList<void>* _tempoMarkers, ReaProject* _proj, double _pos1, double _pos2);
bool IsTempoMarkerInInterval(TempoMarker* _tempoMarker, double _pos1, double _pos2);
bool DuplicateTempoMarkers(WDL_PtrList<void>* _tempoMarkers, ReaProject* _proj, const char* _undoTitle, double _nudgePos);
#endif
7 changes: 7 additions & 0 deletions SnM/SnM_RegionPlaylist.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1867,6 +1867,8 @@ void AppendPasteCropPlaylist(RegionPlaylist* _playlist, const AppendPasteCropPla

WDL_PtrList<void> itemsToKeep;
GetItemsInInterval(&itemsToKeep, rgnpos, rgnend, false);
WDL_PtrList<void> tempoMarkersToKeep;
GetTempoMarkersInInterval(&tempoMarkersToKeep, NULL, rgnpos, rgnend);
// store regions
bool found = false;
for (int k = 0; !found && k<rgns.GetSize(); k++)
Expand All @@ -1891,6 +1893,7 @@ void AppendPasteCropPlaylist(RegionPlaylist* _playlist, const AppendPasteCropPla
for (int k = 0; k < plItem->m_cnt; k++)
{
DupSelItems(NULL, endPos-rgnpos, &itemsToKeep); // overrides the native ApplyNudge()
DuplicateTempoMarkers(&tempoMarkersToKeep, NULL, NULL, endPos - rgnpos);
Copy link
Member

Choose a reason for hiding this comment

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

Could probably write the new tempo envelope only once after (or before?) processing all regions instead of once for every region.

endPos += (rgnend-rgnpos);
}

Expand Down Expand Up @@ -1995,6 +1998,8 @@ void AppendPasteCropPlaylist(RegionPlaylist* _playlist, const AppendPasteCropPla

Main_OnCommand(40296, 0); // select all tracks
Main_OnCommand(40210, 0); // copy tracks
WDL_PtrList<void> tempoMarkers;
GetAllTempoMarkers(&tempoMarkers, NULL);

PreventUIRefresh(-1);

Expand Down Expand Up @@ -2032,6 +2037,8 @@ void AppendPasteCropPlaylist(RegionPlaylist* _playlist, const AppendPasteCropPla
g_pls.Get()->Add(dupPlaylist);
g_pls.Get()->m_editId = 0;

DuplicateTempoMarkers(&tempoMarkers, NULL, NULL, 0);

PreventUIRefresh(-1);
SNM_UIRefresh(NULL);

Expand Down