From 77d69e2db95947a7bd5c82fc9264d9dd70ae5de8 Mon Sep 17 00:00:00 2001 From: IanCaio Date: Tue, 13 Oct 2020 10:39:06 -0300 Subject: [PATCH 01/61] First commit This commit introduces the use of AutomationNodes instead of just floats for keeping the automation values. The AutomationNodes will give more flexibility when it comes to storing extra information about the values (and make it possible to have multiple progression types in the future). Now the tangents are stored on the node, requiring one less timeMap on the automation pattern. Some methods had to be changed for that, since before they used const_iterator, which wouldn't allow us to change the tangent values. TODO: - Check TODO comments that were added in places of the code I had some doubts about. - Maybe change the timeMap to hold pointers to AutomationNodes and not actual instances. - In some pieces of the code, I check if the AutomationNode already exists before setting its value or creating another node. I think that's unnecessary since if I create another node and assign it to the current existing one it could just clone its value. (That would not be valid for pointers to AutomationNodes, so that's something to consider when deciding between the two). - I still didn't find a good solution for renaming QMap::iterator method from "value()" to something else, so now we have lines that look a bit odd like "it.value().getValue()", because value() is actually returning the node, and getValue() is getting the node's automation value. --- include/AutomationPattern.h | 52 +++++++++--- include/InlineAutomation.h | 4 +- src/core/AutomationPattern.cpp | 114 ++++++++++++++++++--------- src/gui/AutomationPatternView.cpp | 18 ++--- src/gui/editors/AutomationEditor.cpp | 34 ++++---- src/gui/editors/PianoRoll.cpp | 2 +- 6 files changed, 146 insertions(+), 78 deletions(-) diff --git a/include/AutomationPattern.h b/include/AutomationPattern.h index cad9d0a1d00..ea439b4934f 100644 --- a/include/AutomationPattern.h +++ b/include/AutomationPattern.h @@ -38,6 +38,43 @@ class MidiTime; +class AutomationNode +{ +public: + AutomationNode(); + AutomationNode( float value ); + + inline float getValue() + { + return m_value; + } + inline const float getValue() const + { + return m_value; + } + inline void setValue( float value ) + { + m_value = value; + } + + inline float getTangent() + { + return m_tangent; + } + inline const float getTangent() const + { + return m_tangent; + } + inline void setTangent( float tangent ) + { + m_tangent = tangent; + } + +private: + float m_value; + float m_tangent; // slope at each point for calculating spline +} ; + class LMMS_EXPORT AutomationPattern : public TrackContentObject { Q_OBJECT @@ -49,7 +86,7 @@ class LMMS_EXPORT AutomationPattern : public TrackContentObject CubicHermiteProgression } ; - typedef QMap timeMap; + typedef QMap timeMap; typedef QVector > objectVector; AutomationPattern( AutomationTrack * _auto_track ); @@ -109,16 +146,6 @@ class LMMS_EXPORT AutomationPattern : public TrackContentObject return m_timeMap; } - inline const timeMap & getTangents() const - { - return m_tangents; - } - - inline timeMap & getTangents() - { - return m_tangents; - } - inline float getMin() const { return firstObject()->minValue(); @@ -170,7 +197,7 @@ public slots: private: void cleanObjects(); void generateTangents(); - void generateTangents( timeMap::const_iterator it, int numToGenerate ); + void generateTangents( timeMap::iterator it, int numToGenerate ); float valueAt( timeMap::const_iterator v, int offset ) const; AutomationTrack * m_autoTrack; @@ -178,7 +205,6 @@ public slots: objectVector m_objects; timeMap m_timeMap; // actual values timeMap m_oldTimeMap; // old values for storing the values before setDragValue() is called. - timeMap m_tangents; // slope at each point for calculating spline float m_tension; bool m_hasAutomation; ProgressionTypes m_progressionType; diff --git a/include/InlineAutomation.h b/include/InlineAutomation.h index 431ecbc81be..5f4ad8d68f2 100644 --- a/include/InlineAutomation.h +++ b/include/InlineAutomation.h @@ -57,8 +57,8 @@ class InlineAutomation : public FloatModel, public sharedObject // of model which is going to be saved anyways if( isAtInitValue() && m_autoPattern->getTimeMap().size() == 1 && - m_autoPattern->getTimeMap().keys().first() == 0 && - m_autoPattern->getTimeMap().values().first() == value() ) + m_autoPattern->getTimeMap().begin().key() == 0 && + m_autoPattern->getTimeMap().begin().value().getValue() == value() ) { return false; } diff --git a/src/core/AutomationPattern.cpp b/src/core/AutomationPattern.cpp index 2fd1cea125f..724a26aa016 100644 --- a/src/core/AutomationPattern.cpp +++ b/src/core/AutomationPattern.cpp @@ -82,8 +82,8 @@ AutomationPattern::AutomationPattern( const AutomationPattern & _pat_to_copy ) : for( timeMap::const_iterator it = _pat_to_copy.m_timeMap.begin(); it != _pat_to_copy.m_timeMap.end(); ++it ) { - m_timeMap[it.key()] = it.value(); - m_tangents[it.key()] = _pat_to_copy.m_tangents[it.key()]; + // Copies the automation node (value and tangent) + m_timeMap[ it.key() ] = it.value(); } switch( getTrack()->trackContainer()->type() ) { @@ -213,8 +213,18 @@ MidiTime AutomationPattern::putValue( const MidiTime & time, Note::quantized( time, quantization() ) : time; - m_timeMap[ newTime ] = value; - timeMap::const_iterator it = m_timeMap.find( newTime ); + // If we do have a node for that midi time, set its value + if( m_timeMap.contains( newTime ) ) + { + m_timeMap[ newTime ].setValue( value ); + } + // If we don't have a node, create one + else + { + m_timeMap[ newTime ] = AutomationNode( value ); + } + + timeMap::iterator it = m_timeMap.find( newTime ); // Remove control points that are covered by the new points // quantization value. Control Key to override @@ -246,8 +256,7 @@ void AutomationPattern::removeValue( const MidiTime & time ) cleanObjects(); m_timeMap.remove( time ); - m_tangents.remove( time ); - timeMap::const_iterator it = m_timeMap.lowerBound( time ); + timeMap::iterator it = m_timeMap.lowerBound( time ); if( it != m_timeMap.begin() ) { --it; @@ -304,7 +313,7 @@ MidiTime AutomationPattern::setDragValue( const MidiTime & time, //Restore to the state before it the point were being dragged m_timeMap = m_oldTimeMap; - for( timeMap::const_iterator it = m_timeMap.begin(); it != m_timeMap.end(); ++it ) + for( timeMap::iterator it = m_timeMap.begin(); it != m_timeMap.end(); ++it ) { generateTangents( it, 3 ); } @@ -334,14 +343,15 @@ float AutomationPattern::valueAt( const MidiTime & _time ) const return 0; } + // If we have a node at that time, just return its value if( m_timeMap.contains( _time ) ) { - return m_timeMap[_time]; + return m_timeMap[_time].getValue(); } // lowerBound returns next value with greater key, therefore we take // the previous element to get the current value - timeMap::ConstIterator v = m_timeMap.lowerBound( _time ); + timeMap::const_iterator v = m_timeMap.lowerBound( _time ); if( v == m_timeMap.begin() ) { @@ -349,7 +359,7 @@ float AutomationPattern::valueAt( const MidiTime & _time ) const } if( v == m_timeMap.end() ) { - return (v-1).value(); + return (v-1).value().getValue(); } return valueAt( v-1, _time - (v-1).key() ); @@ -362,13 +372,13 @@ float AutomationPattern::valueAt( timeMap::const_iterator v, int offset ) const { if( m_progressionType == DiscreteProgression || v == m_timeMap.end() ) { - return v.value(); + return v.value().getValue(); } else if( m_progressionType == LinearProgression ) { - float slope = ((v+1).value() - v.value()) / + float slope = ((v+1).value().getValue() - v.value().getValue()) / ((v+1).key() - v.key()); - return v.value() + offset * slope; + return v.value().getValue() + offset * slope; } else /* CubicHermiteProgression */ { @@ -382,12 +392,12 @@ float AutomationPattern::valueAt( timeMap::const_iterator v, int offset ) const // tangents _m1 and _m2 int numValues = ((v+1).key() - v.key()); float t = (float) offset / (float) numValues; - float m1 = (m_tangents[v.key()]) * numValues * m_tension; - float m2 = (m_tangents[(v+1).key()]) * numValues * m_tension; + float m1 = v.value().getTangent() * numValues * m_tension; + float m2 = (v+1).value().getTangent() * numValues * m_tension; - return ( 2*pow(t,3) - 3*pow(t,2) + 1 ) * v.value() + return ( 2*pow(t,3) - 3*pow(t,2) + 1 ) * v.value().getValue() + ( pow(t,3) - 2*pow(t,2) + t) * m1 - + ( -2*pow(t,3) + 3*pow(t,2) ) * (v+1).value() + + ( -2*pow(t,3) + 3*pow(t,2) ) * (v+1).value().getValue() + ( pow(t,3) - pow(t,2) ) * m2; } } @@ -397,7 +407,7 @@ float AutomationPattern::valueAt( timeMap::const_iterator v, int offset ) const float *AutomationPattern::valuesAfter( const MidiTime & _time ) const { - timeMap::ConstIterator v = m_timeMap.lowerBound( _time ); + timeMap::const_iterator v = m_timeMap.lowerBound( _time ); if( v == m_timeMap.end() || (v+1) == m_timeMap.end() ) { return NULL; @@ -420,11 +430,13 @@ float *AutomationPattern::valuesAfter( const MidiTime & _time ) const void AutomationPattern::flipY( int min, int max ) { timeMap tempMap = m_timeMap; - timeMap::ConstIterator iterate = m_timeMap.lowerBound(0); + timeMap::const_iterator iterate = m_timeMap.lowerBound(0); float tempValue = 0; int numPoints = 0; + // TODO: This loop looks really odd. Is there a particular case where iterate+i+1 != m_timeMap.end() + // will be true but iterate + 1 != m_timeMap.end() will be false? for( int i = 0; ( iterate + i + 1 ) != m_timeMap.end() && ( iterate + i ) != m_timeMap.end() ; i++) { numPoints++; @@ -464,10 +476,11 @@ void AutomationPattern::flipX( int length ) { timeMap tempMap; - timeMap::ConstIterator iterate = m_timeMap.lowerBound(0); + timeMap::const_iterator iterate = m_timeMap.lowerBound(0); float tempValue = 0; int numPoints = 0; + // TODO: Same as the comment on AutomationPattern::flipY for( int i = 0; ( iterate + i + 1 ) != m_timeMap.end() && ( iterate + i ) != m_timeMap.end() ; i++) { numPoints++; @@ -486,7 +499,10 @@ void AutomationPattern::flipX( int length ) { tempValue = valueAt( ( iterate + i ).key() ); MidiTime newTime = MidiTime( length - ( iterate + i ).key() ); - tempMap[newTime] = tempValue; + // TODO: Can we be sure newTime will never repeat its value? + // If not we need to check first if the key already has an object + // and just set its value if it does. + tempMap[newTime] = AutomationNode( tempValue ); } } else @@ -504,7 +520,10 @@ void AutomationPattern::flipX( int length ) { newTime = MidiTime( ( iterate + i ).key() ); } - tempMap[newTime] = tempValue; + // TODO: Can we be sure newTime will never repeat its value? + // If not we need to check first if the key already has an object + // and just set its value if it does. + tempMap[newTime] = AutomationNode( tempValue ); } } } @@ -515,7 +534,10 @@ void AutomationPattern::flipX( int length ) tempValue = valueAt( ( iterate + i ).key() ); cleanObjects(); MidiTime newTime = MidiTime( realLength - ( iterate + i ).key() ); - tempMap[newTime] = tempValue; + // TODO: Can we be sure newTime will never repeat its value? + // If not we need to check first if the key already has an object + // and just set its value if it does. + tempMap[newTime] = AutomationNode( tempValue ); } } @@ -544,7 +566,7 @@ void AutomationPattern::saveSettings( QDomDocument & _doc, QDomElement & _this ) { QDomElement element = _doc.createElement( "time" ); element.setAttribute( "pos", it.key() ); - element.setAttribute( "value", it.value() ); + element.setAttribute( "value", it.value().getValue() ); _this.appendChild( element ); } @@ -585,8 +607,17 @@ void AutomationPattern::loadSettings( const QDomElement & _this ) } if( element.tagName() == "time" ) { - m_timeMap[element.attribute( "pos" ).toInt()] - = LocaleHelper::toFloat(element.attribute("value")); + int timeMapPos = element.attribute("pos").toInt(); + float timeMapValue = LocaleHelper::toFloat(element.attribute("value")); + // If we already have an automation node just set its value + if( m_timeMap.contains( timeMapPos ) ) + { + m_timeMap[timeMapPos].setValue( timeMapValue ); + } + else + { + m_timeMap[timeMapPos] = AutomationNode( timeMapValue ); + } } else if( element.tagName() == "object" ) { @@ -807,7 +838,6 @@ void AutomationPattern::resolveAllIDs() void AutomationPattern::clear() { m_timeMap.clear(); - m_tangents.clear(); emit dataChanged(); } @@ -867,12 +897,12 @@ void AutomationPattern::generateTangents() -void AutomationPattern::generateTangents( timeMap::const_iterator it, +void AutomationPattern::generateTangents( timeMap::iterator it, int numToGenerate ) { if( m_timeMap.size() < 2 && numToGenerate > 0 ) { - m_tangents[it.key()] = 0; + it.value().setTangent( 0 ); return; } @@ -880,20 +910,22 @@ void AutomationPattern::generateTangents( timeMap::const_iterator it, { if( it == m_timeMap.begin() ) { - m_tangents[it.key()] = - ( (it+1).value() - (it).value() ) / - ( (it+1).key() - (it).key() ); + it.value().setTangent( + ( (it+1).value().getValue() - (it).value().getValue() ) / + ( (it+1).key() - (it).key() ) + ); } else if( it+1 == m_timeMap.end() ) { - m_tangents[it.key()] = 0; + it.value().setTangent( 0 ); return; } else { - m_tangents[it.key()] = - ( (it+1).value() - (it-1).value() ) / - ( (it+1).key() - (it-1).key() ); + it.value().setTangent( + ( (it+1).value().getValue() - (it-1).value().getValue() ) / + ( (it+1).key() - (it-1).key() ) + ); } it++; } @@ -902,4 +934,14 @@ void AutomationPattern::generateTangents( timeMap::const_iterator it, +AutomationNode::AutomationNode() : + m_value( 0 ), + m_tangent( 0 ) +{ +} +AutomationNode::AutomationNode( float value ) : + m_value( value ), + m_tangent( 0 ) +{ +} diff --git a/src/gui/AutomationPatternView.cpp b/src/gui/AutomationPatternView.cpp index 6daba567b0f..b2e354ee05e 100644 --- a/src/gui/AutomationPatternView.cpp +++ b/src/gui/AutomationPatternView.cpp @@ -317,11 +317,11 @@ void AutomationPatternView::paintEvent( QPaintEvent * ) if( x1 > ( width() - TCO_BORDER_WIDTH ) ) break; if( gradient() ) { - p.fillRect( QRectF( x1, 0.0f, x2 - x1, it.value() ), lin2grad ); + p.fillRect( QRectF( x1, 0.0f, x2 - x1, it.value().getValue() ), lin2grad ); } else { - p.fillRect( QRectF( x1, 0.0f, x2 - x1, it.value() ), col ); + p.fillRect( QRectF( x1, 0.0f, x2 - x1, it.value().getValue() ), col ); } break; } @@ -331,11 +331,11 @@ void AutomationPatternView::paintEvent( QPaintEvent * ) float nextValue; if( m_pat->progressionType() == AutomationPattern::DiscreteProgression ) { - nextValue = it.value(); + nextValue = it.value().getValue(); } else { - nextValue = ( it + 1 ).value(); + nextValue = ( it + 1 ).value().getValue(); } QPainterPath path; @@ -483,15 +483,15 @@ void AutomationPatternView::scaleTimemapToFit( float oldMin, float oldMax ) for( AutomationPattern::timeMap::iterator it = m_pat->m_timeMap.begin(); it != m_pat->m_timeMap.end(); ++it ) { - if( *it < oldMin ) + if( it.value().getValue() < oldMin ) { - *it = oldMin; + it.value().setValue( oldMin ); } - else if( *it > oldMax ) + else if( it.value().getValue() > oldMax ) { - *it = oldMax; + it.value().setValue( oldMax ); } - *it = (*it-oldMin)*(newMax-newMin)/(oldMax-oldMin)+newMin; + it.value().setValue( (it.value().getValue()-oldMin)*(newMax-newMin)/(oldMax-oldMin)+newMin ); } m_pat->generateTangents(); diff --git a/src/gui/editors/AutomationEditor.cpp b/src/gui/editors/AutomationEditor.cpp index b64cea0f5f2..83587de6c66 100644 --- a/src/gui/editors/AutomationEditor.cpp +++ b/src/gui/editors/AutomationEditor.cpp @@ -528,7 +528,7 @@ void AutomationEditor::mousePressEvent( QMouseEvent* mouseEvent ) ( it+1==time_map.end() || pos_ticks <= (it+1).key() ) && ( pos_ticks<= it.key() + MidiTime::ticksPerBar() *4 / m_ppb ) && - ( level == it.value() || mouseEvent->button() == Qt::RightButton ) ) + ( level == it.value().getValue() || mouseEvent->button() == Qt::RightButton ) ) { break; } @@ -807,7 +807,7 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) if( pos_ticks >= it.key() && ( it+1==time_map.end() || pos_ticks <= (it+1).key() ) && - level <= it.value() ) + level <= it.value().getValue() ) { break; } @@ -996,9 +996,9 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) } new_selValuesForMove[ m_pattern->putValue( new_value_pos, - it.value () + level_diff, + it.value().getValue() + level_diff, false )] - = it.value() + level_diff; + = it.value().getValue() + level_diff; } m_selValuesForMove = new_selValuesForMove; @@ -1122,7 +1122,7 @@ inline void AutomationEditor::drawCross( QPainter & p ) inline void AutomationEditor::drawAutomationPoint( QPainter & p, timeMap::iterator it ) { int x = xCoordOfTick( it.key() ); - int y = yCoordOfLevel( it.value() ); + int y = yCoordOfLevel( it.value().getValue() ); const int outerRadius = qBound( 3, ( m_ppb * AutomationPattern::quantization() ) / 576, 5 ); // man, getting this calculation right took forever p.setPen( QPen( vertexColor().lighter( 200 ) ) ); p.setBrush( QBrush( vertexColor() ) ); @@ -1390,8 +1390,8 @@ void AutomationEditor::paintEvent(QPaintEvent * pe ) is_selected = true; } } - else if( it.value() >= selLevel_start && - it.value() <= selLevel_end && + else if( it.value().getValue() >= selLevel_start && + it.value().getValue() <= selLevel_end && it.key() >= sel_pos_start && it.key() + len_ticks <= sel_pos_end ) { @@ -1403,11 +1403,11 @@ void AutomationEditor::paintEvent(QPaintEvent * pe ) float nextValue; if( m_pattern->progressionType() == AutomationPattern::DiscreteProgression ) { - nextValue = it.value(); + nextValue = it.value().getValue(); } else { - nextValue = ( it + 1 ).value(); + nextValue = ( it + 1 ).value().getValue(); } p.setRenderHints( QPainter::Antialiasing, true ); @@ -1438,7 +1438,7 @@ void AutomationEditor::paintEvent(QPaintEvent * pe ) // TODO: Find out if the section after the last control // point is able to be selected and if so set this // boolean correctly - drawLevelTick( p, i, it.value()); ////NEEDS Change in CSS:, false ); + drawLevelTick( p, i, it.value().getValue()); ////NEEDS Change in CSS:, false ); } // Draw circle(the last one) drawAutomationPoint(p, it); @@ -1622,7 +1622,7 @@ void AutomationEditor::centerTopBottomScroll() { // set the position to the inverted value ((max + min) - value) // If we set just (max - value), we're off by m_pattern's minimum - pos = m_pattern->getMax() + m_pattern->getMin() - static_cast(time_map.begin().value()); + pos = m_pattern->getMax() + m_pattern->getMin() - static_cast(time_map.begin().value().getValue()); } } m_topBottomScroll->setValue(pos); @@ -1940,12 +1940,12 @@ void AutomationEditor::selectAll() timeMap::iterator it = time_map.begin(); m_selectStartTick = 0; m_selectedTick = m_pattern->length(); - m_selectStartLevel = it.value(); + m_selectStartLevel = it.value().getValue(); m_selectedLevels = 1; while( ++it != time_map.end() ) { - const float level = it.value(); + const float level = it.value().getValue(); if( level < m_selectStartLevel ) { // if we move start-level down, we have to add @@ -1996,7 +1996,7 @@ void AutomationEditor::getSelectedValues( timeMap & selected_values ) //TODO: Add constant tick_t len_ticks = MidiTime::ticksPerBar() / 16; - float level = it.value(); + float level = it.value().getValue(); tick_t pos_ticks = it.key(); if( level >= selLevel_start && level <= selLevel_end && @@ -2023,7 +2023,7 @@ void AutomationEditor::copySelectedValues() for( timeMap::iterator it = selected_values.begin(); it != selected_values.end(); ++it ) { - m_valuesToCopy[it.key()] = it.value(); + m_valuesToCopy[it.key()] = AutomationNode( it.value().getValue() ) ; } TextFloat::displayMessage( tr( "Values copied" ), tr( "All selected values were copied to the " @@ -2056,7 +2056,7 @@ void AutomationEditor::cutSelectedValues() for( timeMap::iterator it = selected_values.begin(); it != selected_values.end(); ++it ) { - m_valuesToCopy[it.key()] = it.value(); + m_valuesToCopy[it.key()] = AutomationNode( it.value().getValue() ); m_pattern->removeValue( it.key() ); } } @@ -2078,7 +2078,7 @@ void AutomationEditor::pasteValues() it != m_valuesToCopy.end(); ++it ) { m_pattern->putValue( it.key() + m_currentPosition, - it.value() ); + it.value().getValue() ); } // we only have to do the following lines if we pasted at diff --git a/src/gui/editors/PianoRoll.cpp b/src/gui/editors/PianoRoll.cpp index 8f1a827a95e..854f64ae487 100644 --- a/src/gui/editors/PianoRoll.cpp +++ b/src/gui/editors/PianoRoll.cpp @@ -921,7 +921,7 @@ void PianoRoll::drawDetuningInfo( QPainter & _p, const Note * _n, int _x, int pos_ticks = it.key(); int pos_x = _x + pos_ticks * m_ppb / MidiTime::ticksPerBar(); - const float level = it.value(); + const float level = it.value().getValue(); int pos_y = middle_y - level * m_keyLineHeight; From fb2107dca168593b321e9de073e480a1b2c4f83d Mon Sep 17 00:00:00 2001 From: IanCaio Date: Wed, 14 Oct 2020 16:11:52 -0300 Subject: [PATCH 02/61] Implements inValue/outValue on Automation Nodes This commit is a big change in comparison to the previous one. Automation nodes now have a inValue and outValue instead of a single value. The inValue represents the core value of the node, which is used for incoming progressions. The outValue represents the value of the node for progressions starting from that node on. In practice, the true value of the node is the inValue and outValue represents a way of creating discrete jumps in a node's position. By default inValue and outValue are the same. The user will then be allowed to change the outValue to create those discrete jumps. Because their values might be different, we now need two tangents for a single node: One for the curve coming before the node and one for the curve coming after the node. So instead of a single tangent variable, we now have inTangent and outTangent. If inValue and outValue are the same, so are inTangent and outTangent, but if they are different both are calculated according to the curve before and after the node. On the Automation Editor, the inValue of the node is represented by our default blue circle, while the outValue is represented by a red circle with 80% alpha (we should add a variable to the Automation Editor class to hold the color of this second circle in the future). Lots of comments were added on the modified files to explain the existing methods where changes were required (not only explaining the logic of the methods but the reason behind using inValue or outValue on them). Lots of TODO comments were also added as placeholders for changes that could or should be done before the finishing of this PR (or after it). For now only the logic for the nodes was added, but there's still no way for the user to change outValues (on the next commit a small placeholder shortcut will be added to do that for testing purposes). The drawing of the notes detuning on the piano roll was updated to account for the discrete jumps. The drawing of the pattern TCO was updated to account for the discrete jumps. The drawing of the pattern on the AutomationEditor was updated to account for the discrete jumps. IMPORTANT: - There were changes to the loadSettings and saveSettings of AutomationPatterns, to account for inValues and outValues, but I didn't create an upgrade routine yet. Some behavior that is important to mention: - Most operations on nodes (drawing, moving, X/Y flipping, and even selection copy/paste, apparently not fully finished) ignore the outValue, basically reseting it to the inValue. So when an user moves a node with a discrete jump, for example, that discrete jump will be lost and the user will need to set it again. Obviously in the future we might want to keep that information, but that isn't a critical issue, just a behavior that can be improved later by upgrading the code. - Later on we might want to connect a signal to the AutomationNode class, so it calls generateTangents when node data is changed. - When an object is disconnected from an automation pattern and it has to rescale the values so they fit the new range of values (max and min) the outValue is reset, meaning all discrete jumps are lost. This behavior will be changed in the future. Things that I think are also important noting: - Mainly in the src/gui/editor/AutomationEditor.cpp file there were lots of codes that apparently are related to a feature that is not yet finished (moving/cutting/copying/pasting selections of automation values). This doesn't sound good unless it's currently being worked on. I tried my best to update the current code to comply to the use of AutomationNodes so their developing can be picked up from a unbroken state. As with other operations involving AutomationNodes, they only account for the inValue discarding any discrete jumps that were present. - I added comments on some logic that seemed flawed in the src/gui/editor/AutomationEditor.cpp file so it can be reviewed. It's beyond the scope of this PR, but since I had to read and change a lot on that file I thought it was pertinent to at least comment those observations. --- include/AutomationPattern.h | 62 +++++++++--- include/InlineAutomation.h | 3 +- src/core/AutomationPattern.cpp | 144 ++++++++++++++++++--------- src/gui/AutomationPatternView.cpp | 34 +++++-- src/gui/editors/AutomationEditor.cpp | 78 +++++++++++---- src/gui/editors/PianoRoll.cpp | 72 ++++++++++---- 6 files changed, 283 insertions(+), 110 deletions(-) diff --git a/include/AutomationPattern.h b/include/AutomationPattern.h index ea439b4934f..a4b85cabac9 100644 --- a/include/AutomationPattern.h +++ b/include/AutomationPattern.h @@ -43,36 +43,70 @@ class AutomationNode public: AutomationNode(); AutomationNode( float value ); + AutomationNode( float inValue, float outValue ); - inline float getValue() + inline float getInValue() { - return m_value; + return m_inValue; } - inline const float getValue() const + inline const float getInValue() const { - return m_value; + return m_inValue; } - inline void setValue( float value ) + inline void setInValue( float value ) { - m_value = value; + m_inValue = value; } - inline float getTangent() + inline float getOutValue() { - return m_tangent; + return m_outValue; } - inline const float getTangent() const + inline const float getOutValue() const { - return m_tangent; + return m_outValue; } - inline void setTangent( float tangent ) + inline void setOutValue( float value ) { - m_tangent = tangent; + m_outValue = value; + } + + inline float getInTangent() + { + return m_inTangent; + } + inline const float getInTangent() const + { + return m_inTangent; + } + inline void setInTangent( float tangent ) + { + m_inTangent = tangent; + } + + inline float getOutTangent() + { + return m_outTangent; + } + inline const float getOutTangent() const + { + return m_outTangent; + } + inline void setOutTangent( float tangent ) + { + m_outTangent = tangent; } private: - float m_value; - float m_tangent; // slope at each point for calculating spline + float m_inValue; + float m_outValue; + + // Slope at each point for calculating spline + // We might have discrete jumps between curves, so we possibly have + // two different tangents for each side of the curve. If inValue and + // outValue are equal, inTangent and outTangent are equal too. + float m_inTangent; + float m_outTangent; } ; class LMMS_EXPORT AutomationPattern : public TrackContentObject diff --git a/include/InlineAutomation.h b/include/InlineAutomation.h index 5f4ad8d68f2..b0425fac579 100644 --- a/include/InlineAutomation.h +++ b/include/InlineAutomation.h @@ -58,7 +58,8 @@ class InlineAutomation : public FloatModel, public sharedObject if( isAtInitValue() && m_autoPattern->getTimeMap().size() == 1 && m_autoPattern->getTimeMap().begin().key() == 0 && - m_autoPattern->getTimeMap().begin().value().getValue() == value() ) + // TODO: Do we really care about outValue if we only have one node? I'm assuming no. + m_autoPattern->getTimeMap().begin().value().getInValue() == value() ) { return false; } diff --git a/src/core/AutomationPattern.cpp b/src/core/AutomationPattern.cpp index 724a26aa016..b313e8d94a0 100644 --- a/src/core/AutomationPattern.cpp +++ b/src/core/AutomationPattern.cpp @@ -82,7 +82,7 @@ AutomationPattern::AutomationPattern( const AutomationPattern & _pat_to_copy ) : for( timeMap::const_iterator it = _pat_to_copy.m_timeMap.begin(); it != _pat_to_copy.m_timeMap.end(); ++it ) { - // Copies the automation node (value and tangent) + // Copies the automation node (in/out values and in/out tangents) m_timeMap[ it.key() ] = it.value(); } switch( getTrack()->trackContainer()->type() ) @@ -202,6 +202,8 @@ void AutomationPattern::updateLength() +// For now, when creating a node, the inValue and outValue will match. After the node is +// created, the user will then be able to drag the outValue to make discrete jumps MidiTime AutomationPattern::putValue( const MidiTime & time, const float value, const bool quantPos, @@ -216,7 +218,8 @@ MidiTime AutomationPattern::putValue( const MidiTime & time, // If we do have a node for that midi time, set its value if( m_timeMap.contains( newTime ) ) { - m_timeMap[ newTime ].setValue( value ); + m_timeMap[ newTime ].setInValue( value ); + m_timeMap[ newTime ].setOutValue( value ); } // If we don't have a node, create one else @@ -346,7 +349,8 @@ float AutomationPattern::valueAt( const MidiTime & _time ) const // If we have a node at that time, just return its value if( m_timeMap.contains( _time ) ) { - return m_timeMap[_time].getValue(); + // When the time is exactly the node's time, we want the inValue + return m_timeMap[_time].getInValue(); } // lowerBound returns next value with greater key, therefore we take @@ -359,7 +363,8 @@ float AutomationPattern::valueAt( const MidiTime & _time ) const } if( v == m_timeMap.end() ) { - return (v-1).value().getValue(); + // When the time is after the last node, we want the outValue of it + return (v-1).value().getOutValue(); } return valueAt( v-1, _time - (v-1).key() ); @@ -368,17 +373,25 @@ float AutomationPattern::valueAt( const MidiTime & _time ) const +// This method will get the value at a offset from a node, so we use the outValue of +// that node and the inValue of the next node to the calculations. This assumes that offset +// will not be zero, because when the midi time given to AutomationPattern::valueAt(MidiTime) +// matches a node's position, that node's value will be returned and this method won't be even +// called. +// TODO: If in the future we want to be able to call this method manually with offset = 0, we +// need to account for that with a simple conditional at the beginning that just returns the +// node's inValue. float AutomationPattern::valueAt( timeMap::const_iterator v, int offset ) const { if( m_progressionType == DiscreteProgression || v == m_timeMap.end() ) { - return v.value().getValue(); + return v.value().getOutValue(); } else if( m_progressionType == LinearProgression ) { - float slope = ((v+1).value().getValue() - v.value().getValue()) / + float slope = ((v+1).value().getInValue() - v.value().getOutValue()) / ((v+1).key() - v.key()); - return v.value().getValue() + offset * slope; + return v.value().getOutValue() + offset * slope; } else /* CubicHermiteProgression */ { @@ -392,12 +405,12 @@ float AutomationPattern::valueAt( timeMap::const_iterator v, int offset ) const // tangents _m1 and _m2 int numValues = ((v+1).key() - v.key()); float t = (float) offset / (float) numValues; - float m1 = v.value().getTangent() * numValues * m_tension; - float m2 = (v+1).value().getTangent() * numValues * m_tension; + float m1 = v.value().getOutTangent() * numValues * m_tension; + float m2 = (v+1).value().getInTangent() * numValues * m_tension; - return ( 2*pow(t,3) - 3*pow(t,2) + 1 ) * v.value().getValue() + return ( 2*pow(t,3) - 3*pow(t,2) + 1 ) * v.value().getOutValue() + ( pow(t,3) - 2*pow(t,2) + t) * m1 - + ( -2*pow(t,3) + 3*pow(t,2) ) * (v+1).value().getValue() + + ( -2*pow(t,3) + 3*pow(t,2) ) * (v+1).value().getInValue() + ( pow(t,3) - pow(t,2) ) * m2; } } @@ -427,6 +440,11 @@ float *AutomationPattern::valuesAfter( const MidiTime & _time ) const +// TODO: Because putValue sets both the inValue and outValue to the same value, flipping +// the pattern will ignore outValues that are different (meaning discrete jumps will be lost). +// If we want them to be kept we need to update the logic so they are accounted for. For now +// this behavior is acceptable (discrete jumps will likely lose their meaning on a flipped +// pattern anyways) void AutomationPattern::flipY( int min, int max ) { timeMap tempMap = m_timeMap; @@ -472,6 +490,11 @@ void AutomationPattern::flipY() +// TODO: This method sets both the inValue and outValue of the new timeMap to the same value, +// so flipping the pattern will ignore outValues that are different (meaning discrete jumps will be lost). +// If we want them to be kept we need to update the logic so they are accounted for. For now +// this behavior is acceptable (discrete jumps will likely lose their meaning on a flipped +// pattern anyways) void AutomationPattern::flipX( int length ) { timeMap tempMap; @@ -499,9 +522,6 @@ void AutomationPattern::flipX( int length ) { tempValue = valueAt( ( iterate + i ).key() ); MidiTime newTime = MidiTime( length - ( iterate + i ).key() ); - // TODO: Can we be sure newTime will never repeat its value? - // If not we need to check first if the key already has an object - // and just set its value if it does. tempMap[newTime] = AutomationNode( tempValue ); } } @@ -520,9 +540,6 @@ void AutomationPattern::flipX( int length ) { newTime = MidiTime( ( iterate + i ).key() ); } - // TODO: Can we be sure newTime will never repeat its value? - // If not we need to check first if the key already has an object - // and just set its value if it does. tempMap[newTime] = AutomationNode( tempValue ); } } @@ -534,9 +551,6 @@ void AutomationPattern::flipX( int length ) tempValue = valueAt( ( iterate + i ).key() ); cleanObjects(); MidiTime newTime = MidiTime( realLength - ( iterate + i ).key() ); - // TODO: Can we be sure newTime will never repeat its value? - // If not we need to check first if the key already has an object - // and just set its value if it does. tempMap[newTime] = AutomationNode( tempValue ); } } @@ -566,7 +580,9 @@ void AutomationPattern::saveSettings( QDomDocument & _doc, QDomElement & _this ) { QDomElement element = _doc.createElement( "time" ); element.setAttribute( "pos", it.key() ); - element.setAttribute( "value", it.value().getValue() ); + // TODO: Requires project file upgrade!!! + element.setAttribute( "inValue", it.value().getInValue() ); + element.setAttribute( "outValue", it.value().getOutValue() ); _this.appendChild( element ); } @@ -608,16 +624,11 @@ void AutomationPattern::loadSettings( const QDomElement & _this ) if( element.tagName() == "time" ) { int timeMapPos = element.attribute("pos").toInt(); - float timeMapValue = LocaleHelper::toFloat(element.attribute("value")); - // If we already have an automation node just set its value - if( m_timeMap.contains( timeMapPos ) ) - { - m_timeMap[timeMapPos].setValue( timeMapValue ); - } - else - { - m_timeMap[timeMapPos] = AutomationNode( timeMapValue ); - } + // TODO: Requires project file upgrade!!! + float timeMapInValue = LocaleHelper::toFloat(element.attribute("inValue")); + float timeMapOutValue = LocaleHelper::toFloat(element.attribute("outValue")); + + m_timeMap[timeMapPos] = AutomationNode( timeMapInValue, timeMapOutValue ); } else if( element.tagName() == "object" ) { @@ -897,12 +908,17 @@ void AutomationPattern::generateTangents() +// We have two tangents, one for the left side of the node and one for the right side +// of the node (in case we have discrete value jumps in the middle of a curve). +// If the inValue and outValue of a node are the same, consequently the inTangent and +// outTangent values of the node will be the same too. void AutomationPattern::generateTangents( timeMap::iterator it, int numToGenerate ) { if( m_timeMap.size() < 2 && numToGenerate > 0 ) { - it.value().setTangent( 0 ); + it.value().setInTangent( 0 ); + it.value().setOutTangent( 0 ); return; } @@ -910,22 +926,48 @@ void AutomationPattern::generateTangents( timeMap::iterator it, { if( it == m_timeMap.begin() ) { - it.value().setTangent( - ( (it+1).value().getValue() - (it).value().getValue() ) / - ( (it+1).key() - (it).key() ) - ); + // On the first node there's no curve behind it, so we will only calculate the outTangent + // and inTangent will be set to 0. + float tangent = ( (it+1).value().getInValue() - (it).value().getOutValue() ) / + ( (it+1).key() - (it).key() ); + it.value().setInTangent( 0 ); + it.value().setOutTangent( tangent ); } else if( it+1 == m_timeMap.end() ) { - it.value().setTangent( 0 ); + // Previously, the last value's tangent was always set to 0. That logic was kept for both tangents + // of the last node + it.value().setInTangent( 0 ); + it.value().setOutTangent( 0 ); return; } else { - it.value().setTangent( - ( (it+1).value().getValue() - (it-1).value().getValue() ) / - ( (it+1).key() - (it-1).key() ) - ); + // When we are in a node that is in the middle of two other nodes, we need to check if we + // have a discrete jump at this node. If we do not, then we can calculate the tangents normally. + // If we do have a discrete jump, then we have to calculate the tangents differently for each side + // of the curve. + float inTangent; + float outTangent; + if( it.value().getInValue() == it.value().getOutValue() ) + { + inTangent = ( (it+1).value().getInValue() - (it-1).value().getOutValue() ) / + ( (it+1).key() - (it-1).key() ); + it.value().setInTangent( inTangent ); + // inTangent == outTangent in this case + it.value().setOutTangent( inTangent ); + } + else + { + // Calculate the left side of the curve + inTangent = ( (it).value().getInValue() - (it-1).value().getOutValue() ) / + ( (it).key() - (it-1).key() ); + // Calculate the right side of the curve + outTangent = ( (it+1).value().getInValue() - (it).value().getOutValue() ) / + ( (it+1).key() - (it).key() ); + it.value().setInTangent( inTangent ); + it.value().setOutTangent( outTangent ); + } } it++; } @@ -935,13 +977,25 @@ void AutomationPattern::generateTangents( timeMap::iterator it, AutomationNode::AutomationNode() : - m_value( 0 ), - m_tangent( 0 ) + m_inValue( 0 ), + m_outValue( 0 ), + m_inTangent( 0 ), + m_outTangent( 0 ) { } AutomationNode::AutomationNode( float value ) : - m_value( value ), - m_tangent( 0 ) + m_inValue( value ), + m_outValue( value ), + m_inTangent( 0 ), + m_outTangent( 0 ) +{ +} + +AutomationNode::AutomationNode( float inValue, float outValue ) : + m_inValue( inValue ), + m_outValue( outValue ), + m_inTangent( 0 ), + m_outTangent( 0 ) { } diff --git a/src/gui/AutomationPatternView.cpp b/src/gui/AutomationPatternView.cpp index b2e354ee05e..aa05f191e14 100644 --- a/src/gui/AutomationPatternView.cpp +++ b/src/gui/AutomationPatternView.cpp @@ -315,27 +315,34 @@ void AutomationPatternView::paintEvent( QPaintEvent * ) const float x1 = x_base + it.key() * ppTick; const float x2 = (float)( width() - TCO_BORDER_WIDTH ); if( x1 > ( width() - TCO_BORDER_WIDTH ) ) break; + // We are drawing the space after the last node, so we use the outValue if( gradient() ) { - p.fillRect( QRectF( x1, 0.0f, x2 - x1, it.value().getValue() ), lin2grad ); + p.fillRect( QRectF( x1, 0.0f, x2 - x1, it.value().getOutValue() ), lin2grad ); } else { - p.fillRect( QRectF( x1, 0.0f, x2 - x1, it.value().getValue() ), col ); + p.fillRect( QRectF( x1, 0.0f, x2 - x1, it.value().getOutValue() ), col ); } break; } float *values = m_pat->valuesAfter( it.key() ); + // We are creating a path to draw a polygon representing the values between two + // nodes. When we have two nodes with discrete progression, we will basically have + // a rectangle with the outValue of the first node (that's why nextValue will match + // the outValue of the current node). When we have nodes with linear or cubic progression + // the value of the end of the shape between the two nodes will be the inValue of + // the next node. float nextValue; if( m_pat->progressionType() == AutomationPattern::DiscreteProgression ) { - nextValue = it.value().getValue(); + nextValue = it.value().getOutValue(); } else { - nextValue = ( it + 1 ).value().getValue(); + nextValue = ( it + 1 ).value().getInValue(); } QPainterPath path; @@ -480,18 +487,27 @@ void AutomationPatternView::scaleTimemapToFit( float oldMin, float oldMax ) return; } + // TODO: Currently when rescaling the timeMap values to fit the new range of values (newMin and newMax) + // only the inValue is being considered and the outValue is being reset to the inValue (so discrete jumps + // are discarded). Possibly later we will want discrete jumps to be maintained so we will need to upgrade + // the logic to account for them. for( AutomationPattern::timeMap::iterator it = m_pat->m_timeMap.begin(); it != m_pat->m_timeMap.end(); ++it ) { - if( it.value().getValue() < oldMin ) + // If the values are out of the previous range, fix them so they are + // between oldMin and oldMax. + if( it.value().getInValue() < oldMin ) { - it.value().setValue( oldMin ); + it.value().setInValue( oldMin ); } - else if( it.value().getValue() > oldMax ) + else if( it.value().getInValue() > oldMax ) { - it.value().setValue( oldMax ); + it.value().setInValue( oldMax ); } - it.value().setValue( (it.value().getValue()-oldMin)*(newMax-newMin)/(oldMax-oldMin)+newMin ); + // Calculate what the value would be proportionally in the new range + it.value().setInValue( (it.value().getInValue()-oldMin)*(newMax-newMin)/(oldMax-oldMin)+newMin ); + // Read earlier TODO comment: For now I'm discarding the discrete jumps during the rescaling + it.value().setOutValue( it.value().getInValue() ); } m_pat->generateTangents(); diff --git a/src/gui/editors/AutomationEditor.cpp b/src/gui/editors/AutomationEditor.cpp index 83587de6c66..cca9a5edd1e 100644 --- a/src/gui/editors/AutomationEditor.cpp +++ b/src/gui/editors/AutomationEditor.cpp @@ -524,11 +524,16 @@ void AutomationEditor::mousePressEvent( QMouseEvent* mouseEvent ) { // and check whether the user clicked on an // existing value + // When handling nodes on the editor, we consider their inValue + // TODO: This logic doesn't look good. It only considers that we clicked a node if + // we click between the position of the node and 4 pixels ahead, and EXACTLY + // on the node's level. Is that what we want? We don't notice it because later + // another node is created on the same position and we start handling it instead. if( pos_ticks >= it.key() && ( it+1==time_map.end() || pos_ticks <= (it+1).key() ) && ( pos_ticks<= it.key() + MidiTime::ticksPerBar() *4 / m_ppb ) && - ( level == it.value().getValue() || mouseEvent->button() == Qt::RightButton ) ) + ( level == it.value().getInValue() || mouseEvent->button() == Qt::RightButton ) ) { break; } @@ -804,10 +809,14 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) { // and check whether the cursor is over an // existing value + // When handling nodes on the editor, we consider their inValue + // TODO: That logic looks bad too. It just checks whether the mouse is between + // two nodes, and in the same level as the first one. You can see it's different + // from the last similar code. if( pos_ticks >= it.key() && ( it+1==time_map.end() || pos_ticks <= (it+1).key() ) && - level <= it.value().getValue() ) + level <= it.value().getInValue() ) { break; } @@ -994,11 +1003,14 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) new_value_pos = MidiTime( value_bar, value_ticks ); } + // When moving selections (is that feature finished?) the outValue will + // be discarded. + // TODO: Maybe account for the outValue when moving selections? new_selValuesForMove[ m_pattern->putValue( new_value_pos, - it.value().getValue() + level_diff, + it.value().getInValue() + level_diff, false )] - = it.value().getValue() + level_diff; + = it.value().getInValue() + level_diff; } m_selValuesForMove = new_selValuesForMove; @@ -1122,11 +1134,18 @@ inline void AutomationEditor::drawCross( QPainter & p ) inline void AutomationEditor::drawAutomationPoint( QPainter & p, timeMap::iterator it ) { int x = xCoordOfTick( it.key() ); - int y = yCoordOfLevel( it.value().getValue() ); + int y = yCoordOfLevel( it.value().getInValue() ); const int outerRadius = qBound( 3, ( m_ppb * AutomationPattern::quantization() ) / 576, 5 ); // man, getting this calculation right took forever p.setPen( QPen( vertexColor().lighter( 200 ) ) ); p.setBrush( QBrush( vertexColor() ) ); p.drawEllipse( x - outerRadius, y - outerRadius, outerRadius * 2, outerRadius * 2 ); + + // Draws another ellipse for the outValue + // TODO: Use some color defined on the Automation Editor class. This is just for testing purposes + y = yCoordOfLevel( it.value().getOutValue() ); + p.setPen( QPen( QColor( 255, 0, 0, 80 ).lighter( 200 ) ) ); + p.setBrush( QBrush( QColor( 255, 0, 0, 80 ) ) ); + p.drawEllipse( x - outerRadius, y - outerRadius, outerRadius * 2, outerRadius * 2 ); } @@ -1390,8 +1409,8 @@ void AutomationEditor::paintEvent(QPaintEvent * pe ) is_selected = true; } } - else if( it.value().getValue() >= selLevel_start && - it.value().getValue() <= selLevel_end && + else if( it.value().getInValue() >= selLevel_start && + it.value().getInValue() <= selLevel_end && it.key() >= sel_pos_start && it.key() + len_ticks <= sel_pos_end ) { @@ -1400,24 +1419,30 @@ void AutomationEditor::paintEvent(QPaintEvent * pe ) float *values = m_pattern->valuesAfter( it.key() ); + // We are creating a path to draw a polygon representing the values between two + // nodes. When we have two nodes with discrete progression, we will basically have + // a rectangle with the outValue of the first node (that's why nextValue will match + // the outValue of the current node). When we have nodes with linear or cubic progression + // the value of the end of the shape between the two nodes will be the inValue of + // the next node. float nextValue; if( m_pattern->progressionType() == AutomationPattern::DiscreteProgression ) { - nextValue = it.value().getValue(); + nextValue = it.value().getOutValue(); } else { - nextValue = ( it + 1 ).value().getValue(); + nextValue = ( it + 1 ).value().getInValue(); } p.setRenderHints( QPainter::Antialiasing, true ); QPainterPath path; path.moveTo( QPointF( xCoordOfTick( it.key() ), yCoordOfLevel( 0 ) ) ); for( int i = 0; i < ( it + 1 ).key() - it.key(); i++ ) - { path.lineTo( QPointF( xCoordOfTick( it.key() + i ), yCoordOfLevel( values[i] ) ) ); + { + path.lineTo( QPointF( xCoordOfTick( it.key() + i ), yCoordOfLevel( values[i] ) ) ); //NEEDS Change in CSS //drawLevelTick( p, it.key() + i, values[i], is_selected ); - } path.lineTo( QPointF( xCoordOfTick( ( it + 1 ).key() ), yCoordOfLevel( nextValue ) ) ); path.lineTo( QPointF( xCoordOfTick( ( it + 1 ).key() ), yCoordOfLevel( 0 ) ) ); @@ -1435,10 +1460,12 @@ void AutomationEditor::paintEvent(QPaintEvent * pe ) for( int i = it.key(), x = xCoordOfTick( i ); x <= width(); i++, x = xCoordOfTick( i ) ) { + // Draws the rectangle representing the value after the last node (for + // that reason we use outValue). // TODO: Find out if the section after the last control // point is able to be selected and if so set this // boolean correctly - drawLevelTick( p, i, it.value().getValue()); ////NEEDS Change in CSS:, false ); + drawLevelTick( p, i, it.value().getOutValue()); ////NEEDS Change in CSS:, false ); } // Draw circle(the last one) drawAutomationPoint(p, it); @@ -1607,7 +1634,7 @@ void AutomationEditor::drawLevelTick(QPainter & p, int tick, float value) -// center the vertical scroll position on the first object's value +// Center the vertical scroll position on the first object's inValue void AutomationEditor::centerTopBottomScroll() { // default to the m_scrollLevel position @@ -1622,7 +1649,7 @@ void AutomationEditor::centerTopBottomScroll() { // set the position to the inverted value ((max + min) - value) // If we set just (max - value), we're off by m_pattern's minimum - pos = m_pattern->getMax() + m_pattern->getMin() - static_cast(time_map.begin().value().getValue()); + pos = m_pattern->getMax() + m_pattern->getMin() - static_cast(time_map.begin().value().getInValue()); } } m_topBottomScroll->setValue(pos); @@ -1927,6 +1954,9 @@ void AutomationEditor::setTension() +// The real node value is inValue, so that's what we care about when creating +// a selection that spans several nodes. If the selected area covers a node's +// inValue it is part of the selection. void AutomationEditor::selectAll() { QMutexLocker m( &m_patternMutex ); @@ -1940,12 +1970,12 @@ void AutomationEditor::selectAll() timeMap::iterator it = time_map.begin(); m_selectStartTick = 0; m_selectedTick = m_pattern->length(); - m_selectStartLevel = it.value().getValue(); + m_selectStartLevel = it.value().getInValue(); m_selectedLevels = 1; while( ++it != time_map.end() ) { - const float level = it.value().getValue(); + const float level = it.value().getInValue(); if( level < m_selectStartLevel ) { // if we move start-level down, we have to add @@ -1996,14 +2026,16 @@ void AutomationEditor::getSelectedValues( timeMap & selected_values ) //TODO: Add constant tick_t len_ticks = MidiTime::ticksPerBar() / 16; - float level = it.value().getValue(); + float level = it.value().getInValue(); tick_t pos_ticks = it.key(); if( level >= selLevel_start && level <= selLevel_end && pos_ticks >= sel_pos_start && pos_ticks + len_ticks <= sel_pos_end ) { - selected_values[it.key()] = level; + // Adds an automation node to our selection timeMap with the selected node's + // inValue (outValue is ignored) + selected_values[it.key()] = AutomationNode( level ); } } } @@ -2023,7 +2055,8 @@ void AutomationEditor::copySelectedValues() for( timeMap::iterator it = selected_values.begin(); it != selected_values.end(); ++it ) { - m_valuesToCopy[it.key()] = AutomationNode( it.value().getValue() ) ; + // Copies the AutomationNode from our selected_values timeMap to m_valuesToCopy + m_valuesToCopy[it.key()] = it.value(); } TextFloat::displayMessage( tr( "Values copied" ), tr( "All selected values were copied to the " @@ -2056,7 +2089,9 @@ void AutomationEditor::cutSelectedValues() for( timeMap::iterator it = selected_values.begin(); it != selected_values.end(); ++it ) { - m_valuesToCopy[it.key()] = AutomationNode( it.value().getValue() ); + // Copies the AutomationNode from our selected_values timeMap to m_valuesToCopy + // and then remove it from the pattern. + m_valuesToCopy[it.key()] = it.value(); m_pattern->removeValue( it.key() ); } } @@ -2077,8 +2112,9 @@ void AutomationEditor::pasteValues() for( timeMap::iterator it = m_valuesToCopy.begin(); it != m_valuesToCopy.end(); ++it ) { + // When pasting we ignore the outValue (which was also ignored during the copy/cut process) m_pattern->putValue( it.key() + m_currentPosition, - it.value().getValue() ); + it.value().getInValue() ); } // we only have to do the following lines if we pasted at diff --git a/src/gui/editors/PianoRoll.cpp b/src/gui/editors/PianoRoll.cpp index 854f64ae487..6a86b964b5f 100644 --- a/src/gui/editors/PianoRoll.cpp +++ b/src/gui/editors/PianoRoll.cpp @@ -912,39 +912,71 @@ void PianoRoll::drawDetuningInfo( QPainter & _p, const Note * _n, int _x, width() - m_whiteKeyWidth, keyAreaBottom() - PR_TOP_MARGIN); + // Draws the lines for the detuning automation. Just as previously, we are still + // drawing cubic hermit curves as lines too. Now account for discrete jumps. int old_x = 0; int old_y = 0; timeMap & map = _n->detuning()->automationPattern()->getTimeMap(); - for( timeMap::ConstIterator it = map.begin(); it != map.end(); ++it ) + for( timeMap::const_iterator it = map.begin(); it != map.end(); ++it ) { - int pos_ticks = it.key(); - int pos_x = _x + pos_ticks * m_ppb / MidiTime::ticksPerBar(); + // Current node values + int cur_ticks = it.key(); + int cur_x = _x + cur_ticks * m_ppb / MidiTime::ticksPerBar(); + const float cur_level = it.value().getInValue(); + int cur_y = middle_y - cur_level * m_keyLineHeight; - const float level = it.value().getValue(); + // First line to represent the inValue of the first node + if( it == map.begin() ) + { + _p.drawLine( cur_x - 1, cur_y, cur_x + 1, cur_y ); + _p.drawLine( cur_x, cur_y - 1, cur_x, cur_y + 1 ); + } + // All subsequent lines will take the outValue of the previous node + // and the inValue of the current node. It will also draw a vertical + // line if there was a discrete jump (from old_x,old_y to pre_x,pre_y) + else + { + // Previous node values (based on outValue). We just calculate + // the y level because the x will be the same as old_x. + const float pre_level = (it-1).value().getOutValue(); + int pre_y = middle_y - pre_level * m_keyLineHeight; - int pos_y = middle_y - level * m_keyLineHeight; + // Draws the line representing the discrete jump if there's one + if( old_y != pre_y ) + { + _p.drawLine( old_x, old_y, old_x, pre_y ); + } - if( old_x != 0 && old_y != 0 ) - { + // Now draw the lines representing the actual progression from one + // node to the other switch( _n->detuning()->automationPattern()->progressionType() ) { - case AutomationPattern::DiscreteProgression: - _p.drawLine( old_x, old_y, pos_x, old_y ); - _p.drawLine( pos_x, old_y, pos_x, pos_y ); - break; - case AutomationPattern::CubicHermiteProgression: /* TODO */ - case AutomationPattern::LinearProgression: - _p.drawLine( old_x, old_y, pos_x, pos_y ); - break; + case AutomationPattern::DiscreteProgression: + _p.drawLine( old_x, pre_y, cur_x, pre_y ); + _p.drawLine( cur_x, pre_y, cur_x, cur_y ); + break; + case AutomationPattern::CubicHermiteProgression: /* TODO */ + case AutomationPattern::LinearProgression: + _p.drawLine( old_x, pre_y, cur_x, cur_y ); + break; } - } - _p.drawLine( pos_x - 1, pos_y, pos_x + 1, pos_y ); - _p.drawLine( pos_x, pos_y - 1, pos_x, pos_y + 1 ); + // If we are in the last node and there's a discrete jump, we draw a + // vertical line representing it + if( (it+1) == map.end() ) + { + const float last_level = it.value().getOutValue(); + if( cur_level != last_level ) + { + int last_y = middle_y - last_level * m_keyLineHeight; + _p.drawLine( cur_x, cur_y, cur_x, last_y ); + } + } + } - old_x = pos_x; - old_y = pos_y; + old_x = cur_x; + old_y = cur_y; } } From 28611b8eec74582d11c681e5d312009e3ff0c92b Mon Sep 17 00:00:00 2001 From: IanCaio Date: Thu, 15 Oct 2020 13:43:36 -0300 Subject: [PATCH 03/61] Fixes and refactor code on AutomationEditor.cpp While implementing the automation nodes, I noticed AutomatinEditor.cpp had some issues regarding flawed logic, code style convention, code that could comply to DRY paradigm, conditions that resulted in Undefined Behavior and unused legacy code. That's probably due to how old some changes are, they probably reflect a much different state of LMMS's code base. To make the transition to automation nodes a better one and avoid having to rework everything later I'm using the fact I have to get in touch with most of this code to try to fix some things. This commit starts refactoring AutomationEditor::mousePressEvent and AutomationEditor::mouseMoveEvent. There are still things to be improved on both but I'll slowly commit them so I can have better versioning control of the PR. Some changes worth noting: - A new action was created in the AutomationEditor class for drawing lines, since its logic was too mixed up with the logic of drawing and dragging a single node. - Changed most variable names to fit the current code style (just very few left to change). - Improved comments explaining the code. - Created a separate method for checking if the mouse position is sitting over an existing node (previously this code was repeated inside the event method and it had flaws on its logic, most of the times returning that the user didn't click a node even when he/she clicked one). Method is called getNodeAt(x,y,r), r being the "radius" to be considered (not actually a radius, the area is actually a square). Some changes that are still planned: - Removing legacy code for features that weren't finished (select and move selection) if it's agreed. - Adding some logic to the DRAW_LINE action so it can be even improved. - Not forgetting the main focus of the PR, adding a way for the user to edit the outValue of nodes. --- include/AutomationEditor.h | 5 + src/gui/editors/AutomationEditor.cpp | 515 ++++++++++++++------------- 2 files changed, 267 insertions(+), 253 deletions(-) diff --git a/include/AutomationEditor.h b/include/AutomationEditor.h index 4813759697f..5905820c5a7 100644 --- a/include/AutomationEditor.h +++ b/include/AutomationEditor.h @@ -131,6 +131,10 @@ public slots: void selectAll(); void getSelectedValues(timeMap & selected_values ); + // Returns the an iterator pointing to the AutomationNode on a radius of + // r pixels of the x,y mouse coordinates, or timeMap.end() if there are none. + timeMap::iterator getNodeAt( int x, int y, int r = 5 ); + void drawLine( int x0, float y0, int x1, float y1 ); void removePoints( int x0, int x1 ); @@ -167,6 +171,7 @@ protected slots: { NONE, MOVE_VALUE, + DRAW_LINE, SELECT_VALUES, MOVE_SELECTION } ; diff --git a/src/gui/editors/AutomationEditor.cpp b/src/gui/editors/AutomationEditor.cpp index cca9a5edd1e..a6b4b40b923 100644 --- a/src/gui/editors/AutomationEditor.cpp +++ b/src/gui/editors/AutomationEditor.cpp @@ -497,162 +497,159 @@ void AutomationEditor::mousePressEvent( QMouseEvent* mouseEvent ) { return; } - if( mouseEvent->y() > TOP_MARGIN ) + + // If we clicked inside the AutomationEditor viewport (where the nodes are represented) + if( mouseEvent->y() > TOP_MARGIN && mouseEvent->x() >= VALUES_WIDTH ) { float level = getLevel( mouseEvent->y() ); - int x = mouseEvent->x(); + // Get the viewport X + int x = mouseEvent->x() - VALUES_WIDTH; - if( x >= VALUES_WIDTH ) - { - // set or move value + // Get tick in which the user clicked + int posTicks = (x * MidiTime::ticksPerBar()/m_ppb) + m_currentPosition; - x -= VALUES_WIDTH; + // Get the time map of current pattern + timeMap & tm = m_pattern->getTimeMap(); - // get tick in which the user clicked - int pos_ticks = x * MidiTime::ticksPerBar() / m_ppb + - m_currentPosition; + // Check if we are clicking over a node (it will be tm.end() if not) + timeMap::iterator clickedNode = getNodeAt(mouseEvent->x(), mouseEvent->y()); - // get time map of current pattern - timeMap & time_map = m_pattern->getTimeMap(); + if (mouseEvent->button() == Qt::LeftButton) + { + m_mouseDownLeft = true; + } + if( mouseEvent->button() == Qt::RightButton ) + { + m_mouseDownRight = true; + } - // will be our iterator in the following loop - timeMap::iterator it = time_map.begin(); + if( mouseEvent->button() == Qt::LeftButton && + m_editMode == DRAW ) + { + m_pattern->addJournalCheckPoint(); - // loop through whole time-map... - while( it != time_map.end() ) + // If we are pressing shift, make a line of nodes from the last draw position + // to where we clicked + if( mouseEvent->modifiers() & Qt::ShiftModifier ) { - // and check whether the user clicked on an - // existing value - // When handling nodes on the editor, we consider their inValue - // TODO: This logic doesn't look good. It only considers that we clicked a node if - // we click between the position of the node and 4 pixels ahead, and EXACTLY - // on the node's level. Is that what we want? We don't notice it because later - // another node is created on the same position and we start handling it instead. - if( pos_ticks >= it.key() && - ( it+1==time_map.end() || - pos_ticks <= (it+1).key() ) && - ( pos_ticks<= it.key() + MidiTime::ticksPerBar() *4 / m_ppb ) && - ( level == it.value().getInValue() || mouseEvent->button() == Qt::RightButton ) ) - { - break; - } - ++it; - } + drawLine( m_drawLastTick, + m_drawLastLevel, + posTicks, level ); - if (mouseEvent->button() == Qt::LeftButton) - { - m_mouseDownLeft = true; - } - if( mouseEvent->button() == Qt::RightButton ) - { - m_mouseDownRight = true; - } + // Beginning of the line of nodes + m_drawLastTick = posTicks; + m_drawLastLevel = level; - // left button?? - if( mouseEvent->button() == Qt::LeftButton && - m_editMode == DRAW ) + // Changes the action to drawing a line of nodes + m_action = DRAW_LINE; + } + else // If we are not pressing shift, we are just creating/moving a node { - m_pattern->addJournalCheckPoint(); - // Connect the dots - if( mouseEvent->modifiers() & Qt::ShiftModifier ) + // If the place we clicked had no nodes + if( clickedNode == tm.end() ) { - drawLine( m_drawLastTick, - m_drawLastLevel, - pos_ticks, level ); - m_mouseDownLeft = false; - } - m_drawLastTick = pos_ticks; - m_drawLastLevel = level; + // We create a new node and start dragging it + MidiTime valuePos( posTicks ); - // did it reach end of map because - // there's no value?? - if( it == time_map.end() ) - { - // then set new value - MidiTime value_pos( pos_ticks ); + // Starts actually moving/draging the node + MidiTime newTime = m_pattern->setDragValue( valuePos, + level, true, mouseEvent->modifiers() & Qt::ControlModifier ); - MidiTime new_time = - m_pattern->setDragValue( value_pos, - level, true, - mouseEvent->modifiers() & - Qt::ControlModifier ); + // Set the iterator to our newly created node so we can use it later + // for the m_moveXOffset calculation + clickedNode = tm.find( newTime ); + } + else // If we clicked over an existing node + { + // Simply start moving/draging it + MidiTime newTime = m_pattern->setDragValue( MidiTime( clickedNode.key() ), + level, true, mouseEvent->modifiers() & Qt::ControlModifier ); - // reset it so that it can be used for - // ops (move, resize) after this - // code-block - it = time_map.find( new_time ); + // We need to update our iterator because setDragValue removes the node that + // is being dragged, so if we don't update it we have a bogus iterator + clickedNode = tm.find( newTime ); } - // move it + // Set the action to MOVE_VALUE so moveMouseEvent() knows we are moving a node m_action = MOVE_VALUE; - int aligned_x = (int)( (float)( ( - it.key() - - m_currentPosition ) * - m_ppb ) / MidiTime::ticksPerBar() ); - m_moveXOffset = x - aligned_x - 1; - // set move-cursor + + // Calculate the offset from the place the mouse click happened in comparison + // to the center of the node + int alignedX = (int)( (float)((clickedNode.key() - m_currentPosition) * m_ppb) / + MidiTime::ticksPerBar() ); + m_moveXOffset = x - alignedX - 1; + + // Set move-cursor QCursor c( Qt::SizeAllCursor ); QApplication::setOverrideCursor( c ); - Engine::getSong()->setModified(); - } - else if( ( mouseEvent->button() == Qt::RightButton && - m_editMode == DRAW ) || - m_editMode == ERASE ) - { - m_drawLastTick = pos_ticks; - m_pattern->addJournalCheckPoint(); - // erase single value - if( it != time_map.end() ) - { - m_pattern->removeValue( it.key() ); - Engine::getSong()->setModified(); - } - m_action = NONE; + // Update the last clicked position so it can be used if we draw a line later + m_drawLastTick = posTicks; + m_drawLastLevel = level; } - else if( mouseEvent->button() == Qt::LeftButton && - m_editMode == SELECT ) - { - // select an area of values - m_selectStartTick = pos_ticks; - m_selectedTick = 0; - m_selectStartLevel = level; - m_selectedLevels = 1; - m_action = SELECT_VALUES; - } - else if( mouseEvent->button() == Qt::RightButton && - m_editMode == SELECT ) + Engine::getSong()->setModified(); + } + else if( ( mouseEvent->button() == Qt::RightButton && + m_editMode == DRAW ) || + m_editMode == ERASE ) + { + m_pattern->addJournalCheckPoint(); + + // Update the last clicked position if we draw a line later + m_drawLastTick = posTicks; + + // If we right-clicked a node, remove it + if( clickedNode != tm.end() ) { - // when clicking right in select-move, we - // switch to move-mode - //m_moveButton->setChecked( true ); + m_pattern->removeValue( clickedNode.key() ); + Engine::getSong()->setModified(); } - else if( mouseEvent->button() == Qt::LeftButton && - m_editMode == MOVE ) - { - m_pattern->addJournalCheckPoint(); - // move selection (including selected values) - // save position where move-process began - m_moveStartTick = pos_ticks; - m_moveStartLevel = level; + m_action = NONE; + } + else if( mouseEvent->button() == Qt::LeftButton && + m_editMode == SELECT ) + { + // select an area of values - m_action = MOVE_SELECTION; + m_selectStartTick = posTicks; + m_selectedTick = 0; + m_selectStartLevel = level; + m_selectedLevels = 1; + m_action = SELECT_VALUES; + } + else if( mouseEvent->button() == Qt::RightButton && + m_editMode == SELECT ) + { + // when clicking right in select-move, we + // switch to move-mode + //m_moveButton->setChecked( true ); + } + else if( mouseEvent->button() == Qt::LeftButton && + m_editMode == MOVE ) + { + m_pattern->addJournalCheckPoint(); + // move selection (including selected values) - Engine::getSong()->setModified(); - } - else if( mouseEvent->button() == Qt::RightButton && - m_editMode == MOVE ) - { - // when clicking right in select-move, we - // switch to draw-mode - //m_drawButton->setChecked( true ); - } + // save position where move-process began + m_moveStartTick = posTicks; + m_moveStartLevel = level; + + m_action = MOVE_SELECTION; - update(); + Engine::getSong()->setModified(); + } + else if( mouseEvent->button() == Qt::RightButton && + m_editMode == MOVE ) + { + // when clicking right in select-move, we + // switch to draw-mode + //m_drawButton->setChecked( true ); } + + update(); } } @@ -683,6 +680,7 @@ void AutomationEditor::mouseReleaseEvent(QMouseEvent * mouseEvent ) { if( m_action == MOVE_VALUE ) { + // Actually apply the value of the node being dragged m_pattern->applyDragValue(); } QApplication::restoreOverrideCursor(); @@ -740,91 +738,85 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) return; } + // If the mouse y position is inside the Automation Editor viewport if( mouseEvent->y() > TOP_MARGIN ) { float level = getLevel( mouseEvent->y() ); - int x = mouseEvent->x(); + // Get the viewport X position where the mouse is at + int x = mouseEvent->x() - VALUES_WIDTH; - x -= VALUES_WIDTH; + // If we are moving a value, we account for the offset from the node we + // had when clicking it in the first place (user might not have clicked exactly + // in the middle of the node) if( m_action == MOVE_VALUE ) { x -= m_moveXOffset; } - int pos_ticks = x * MidiTime::ticksPerBar() / m_ppb + - m_currentPosition; - // m_mouseDownLeft used to prevent drag when drawing line + // Get the X position in ticks + int posTicks = (x * MidiTime::ticksPerBar()/m_ppb) + m_currentPosition; + + // If we are in DRAW mode and the left mouse is down if (m_mouseDownLeft && m_editMode == DRAW) { if( m_action == MOVE_VALUE ) { - // moving value - if( pos_ticks < 0 ) + // If we moved the mouse past the beginning correct the position in ticks + if( posTicks < 0 ) { - pos_ticks = 0; + posTicks = 0; } - drawLine( m_drawLastTick, m_drawLastLevel, - pos_ticks, level ); - - m_drawLastTick = pos_ticks; + m_drawLastTick = posTicks; m_drawLastLevel = level; - // we moved the value so the value has to be - // moved properly according to new starting- - // time in the time map of pattern - m_pattern->setDragValue( MidiTime( pos_ticks ), + // Updates the drag value of the moved node + m_pattern->setDragValue( MidiTime( posTicks ), level, true, mouseEvent->modifiers() & Qt::ControlModifier ); - } - - Engine::getSong()->setModified(); + Engine::getSong()->setModified(); + } + else if( m_action == DRAW_LINE ) + { + // We are drawing a line. For now do nothing (as before), but later logic + // could be added here so the line is updated according to the new mouse position + // until the button is released + } } else if( ( mouseEvent->buttons() & Qt::RightButton && m_editMode == DRAW ) || ( mouseEvent->buttons() & Qt::LeftButton && m_editMode == ERASE ) ) { - // removing automation point - if( pos_ticks < 0 ) + // Removing automation nodes + + // If we moved the mouse past the beginning correct the position in ticks + if( posTicks < 0 ) { - pos_ticks = 0; + posTicks = 0; } - removePoints( m_drawLastTick, pos_ticks ); + + // Removes all values from the last clicked tick up to the current position tick + removePoints( m_drawLastTick, posTicks ); + Engine::getSong()->setModified(); } + // TODO: This is actually not called because we don't have mouseTracking enabled + // for this widget. So we need to either fix or remove that. else if( mouseEvent->buttons() & Qt::NoButton && m_editMode == DRAW ) { // set move- or resize-cursor - // get time map of current pattern - timeMap & time_map = m_pattern->getTimeMap(); + // Get time map of current pattern + timeMap & tm = m_pattern->getTimeMap(); - // will be our iterator in the following loop - timeMap::iterator it = time_map.begin(); - // loop through whole time map... - for( ; it != time_map.end(); ++it ) - { - // and check whether the cursor is over an - // existing value - // When handling nodes on the editor, we consider their inValue - // TODO: That logic looks bad too. It just checks whether the mouse is between - // two nodes, and in the same level as the first one. You can see it's different - // from the last similar code. - if( pos_ticks >= it.key() && - ( it+1==time_map.end() || - pos_ticks <= (it+1).key() ) && - level <= it.value().getInValue() ) - { - break; - } - } + // Check if we have a node on the current mouse position + timeMap::iterator it = getNodeAt(mouseEvent->x(), mouseEvent->y()); - // did it reach end of map because there's - // no value?? - if( it != time_map.end() ) + // If our mouse is hovering over a node, change the cursor + if( it != tm.end() ) { if( QApplication::overrideCursor() ) { @@ -836,8 +828,7 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) } QCursor c( Qt::SizeAllCursor ); - QApplication::setOverrideCursor( - c ); + QApplication::setOverrideCursor( c ); } } else @@ -846,10 +837,8 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) QApplication::setOverrideCursor( c ); } } - else + else // If not, restore cursor { - // the cursor is over no value, so restore - // cursor while( QApplication::overrideCursor() != NULL ) { QApplication::restoreOverrideCursor(); @@ -863,6 +852,10 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) // change size of selection + // TODO: I think this logic is broken. If x is negative, but the m_currentPosition is 0 then + // x is not set to 0 and posTicks was calculated with a negative x value. That summed to + // the m_currentPosition would result in a negative position in ticks. The feature is not being + // used but FIX it later if( x < 0 && m_currentPosition > 0 ) { x = 0; @@ -870,8 +863,7 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) VALUES_WIDTH, mouseEvent->y() ) ) ); if( m_currentPosition >= 4 ) { - m_leftRightScroll->setValue( - m_currentPosition - 4 ); + m_leftRightScroll->setValue( m_currentPosition - 4 ); } else { @@ -883,20 +875,18 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) x = width() - VALUES_WIDTH; QCursor::setPos( mapToGlobal( QPoint( width(), mouseEvent->y() ) ) ); - m_leftRightScroll->setValue( m_currentPosition + - 4 ); + m_leftRightScroll->setValue( m_currentPosition + 4 ); } - // get tick in which the cursor is posated - int pos_ticks = x * MidiTime::ticksPerBar() / m_ppb + - m_currentPosition; + m_selectedTick = posTicks - m_selectStartTick; - m_selectedTick = pos_ticks - m_selectStartTick; if( (int) m_selectStartTick + m_selectedTick < 0 ) { m_selectedTick = -m_selectStartTick; } + m_selectedLevels = level - m_selectStartLevel; + if( level <= m_selectStartLevel ) { --m_selectedLevels; @@ -909,72 +899,55 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) // move selection + selected values // do horizontal move-stuff - int pos_ticks = x * MidiTime::ticksPerBar() / m_ppb + - m_currentPosition; - int ticks_diff = pos_ticks - - m_moveStartTick; + int ticksDiff = posTicks - m_moveStartTick; + if( m_selectedTick > 0 ) { - if( (int) m_selectStartTick + - ticks_diff < 0 ) + if( (int) m_selectStartTick + ticksDiff < 0 ) { - ticks_diff = -m_selectStartTick; + ticksDiff = -m_selectStartTick; } } else { - if( (int) m_selectStartTick + - m_selectedTick + ticks_diff < - 0 ) + if( (int) m_selectStartTick + m_selectedTick + ticksDiff < 0 ) { - ticks_diff = -( - m_selectStartTick + - m_selectedTick ); + ticksDiff = -( m_selectStartTick + m_selectedTick ); } } - m_selectStartTick += ticks_diff; - int bar_diff = ticks_diff / MidiTime::ticksPerBar(); - ticks_diff = ticks_diff % MidiTime::ticksPerBar(); + m_selectStartTick += ticksDiff; + int barDiff = ticksDiff / MidiTime::ticksPerBar(); + ticksDiff = ticksDiff % MidiTime::ticksPerBar(); // do vertical move-stuff - float level_diff = level - m_moveStartLevel; + float levelDiff = level - m_moveStartLevel; if( m_selectedLevels > 0 ) { - if( m_selectStartLevel + level_diff - < m_minLevel ) + if( m_selectStartLevel + levelDiff < m_minLevel ) { - level_diff = m_minLevel - - m_selectStartLevel; + levelDiff = m_minLevel - m_selectStartLevel; } - else if( m_selectStartLevel + m_selectedLevels + - level_diff > m_maxLevel ) + else if( m_selectStartLevel + m_selectedLevels + levelDiff > m_maxLevel ) { - level_diff = m_maxLevel - - m_selectStartLevel - - m_selectedLevels; + levelDiff = m_maxLevel - m_selectStartLevel - m_selectedLevels; } } else { - if( m_selectStartLevel + m_selectedLevels + - level_diff < m_minLevel ) + if( m_selectStartLevel + m_selectedLevels + levelDiff < m_minLevel ) { - level_diff = m_minLevel - - m_selectStartLevel - - m_selectedLevels; + levelDiff = m_minLevel - m_selectStartLevel - m_selectedLevels; } - else if( m_selectStartLevel + level_diff > - m_maxLevel ) + else if( m_selectStartLevel + levelDiff > m_maxLevel ) { - level_diff = m_maxLevel - - m_selectStartLevel; + levelDiff = m_maxLevel - m_selectStartLevel; } } - m_selectStartLevel += level_diff; + m_selectStartLevel += levelDiff; timeMap new_selValuesForMove; for( timeMap::iterator it = m_selValuesForMove.begin(); @@ -983,49 +956,50 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) MidiTime new_value_pos; if( it.key() ) { - int value_bar = - ( it.key() / - MidiTime::ticksPerBar() ) - + bar_diff; - int value_ticks = - ( it.key() % - MidiTime::ticksPerBar() ) - + ticks_diff; + int valueBar = ( it.key() / MidiTime::ticksPerBar() ) + + barDiff; + int valueTicks = ( it.key() % MidiTime::ticksPerBar() ) + + ticksDiff; // ensure value_ticks range - if( value_ticks / MidiTime::ticksPerBar() ) + if( valueTicks / MidiTime::ticksPerBar() ) { - value_bar += value_ticks + valueBar += valueTicks / MidiTime::ticksPerBar(); - value_ticks %= + valueTicks %= MidiTime::ticksPerBar(); } m_pattern->removeValue( it.key() ); - new_value_pos = MidiTime( value_bar, - value_ticks ); + new_value_pos = MidiTime( valueBar, + valueTicks ); } // When moving selections (is that feature finished?) the outValue will // be discarded. // TODO: Maybe account for the outValue when moving selections? new_selValuesForMove[ m_pattern->putValue( new_value_pos, - it.value().getInValue() + level_diff, + it.value().getInValue() + levelDiff, false )] - = it.value().getInValue() + level_diff; + = it.value().getInValue() + levelDiff; } m_selValuesForMove = new_selValuesForMove; - m_moveStartTick = pos_ticks; + m_moveStartTick = posTicks; m_moveStartLevel = level; } } - else + else // If the mouse Y position is above the AutomationEditor viewport { if( mouseEvent->buttons() & Qt::LeftButton && m_editMode == SELECT && m_action == SELECT_VALUES ) { - + // Get the viewport X position of mouse X int x = mouseEvent->x() - VALUES_WIDTH; + + // TODO: I think this logic is broken. If x is negative, but the m_currentPosition is 0 then + // x is not set to 0, and later on posTicks is calculated with a negative x value. That summed to + // the m_currentPosition would result in a negative position in ticks. The feature is not being + // used but FIX it later if( x < 0 && m_currentPosition > 0 ) { x = 0; @@ -1033,8 +1007,7 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) mouseEvent->y() ) ) ); if( m_currentPosition >= 4 ) { - m_leftRightScroll->setValue( - m_currentPosition - 4 ); + m_leftRightScroll->setValue( m_currentPosition - 4 ); } else { @@ -1046,18 +1019,15 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) x = width() - VALUES_WIDTH; QCursor::setPos( mapToGlobal( QPoint( width(), mouseEvent->y() ) ) ); - m_leftRightScroll->setValue( m_currentPosition + - 4 ); + m_leftRightScroll->setValue( m_currentPosition + 4 ); } - // get tick in which the cursor is posated - int pos_ticks = x * MidiTime::ticksPerBar() / m_ppb + - m_currentPosition; + // Get the viewport X position in ticks + int posTicks = (x * MidiTime::ticksPerBar()/m_ppb) + m_currentPosition; + + m_selectedTick = posTicks - m_selectStartTick; - m_selectedTick = pos_ticks - - m_selectStartTick; - if( (int) m_selectStartTick + m_selectedTick < - 0 ) + if( (int) m_selectStartTick + m_selectedTick < 0 ) { m_selectedTick = -m_selectStartTick; } @@ -1069,16 +1039,14 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent ) QCursor::setPos( mapToGlobal( QPoint( mouseEvent->x(), height() - SCROLLBAR_SIZE ) ) ); - m_topBottomScroll->setValue( - m_topBottomScroll->value() + 1 ); + m_topBottomScroll->setValue( m_topBottomScroll->value() + 1 ); level = m_bottomLevel; } else if( level >= m_topLevel ) { QCursor::setPos( mapToGlobal( QPoint( mouseEvent->x(), TOP_MARGIN ) ) ); - m_topBottomScroll->setValue( - m_topBottomScroll->value() - 1 ); + m_topBottomScroll->setValue( m_topBottomScroll->value() - 1 ); level = m_topLevel; } m_selectedLevels = level - m_selectStartLevel; @@ -2274,6 +2242,47 @@ void AutomationEditor::updateTopBottomLevels() +// Get an iterator pointing to the node on a radius of r pixels from mouse position x,y +// Returns timeMap.end() if none is found. +AutomationEditor::timeMap::iterator AutomationEditor::getNodeAt( int x, int y, int r /* = 5 */ ) +{ + // Remove the VALUES_WIDTH from the x position, so we have the actual viewport x + x -= VALUES_WIDTH; + // Convert the x position to the position in ticks + int posTicks = ( x * MidiTime::ticksPerBar()/m_ppb ) + m_currentPosition; + + // Get our pattern timeMap and create a iterator so we can check the nodes + timeMap & tm = m_pattern->getTimeMap(); + timeMap::iterator it = tm.begin(); + + // If there's a node that is inside those coordinates we will store it here + timeMap::iterator node = tm.end(); + + // ticksOffset is the number of ticks that match "r" pixels + int ticksOffset = MidiTime::ticksPerBar() * r /m_ppb; + + while( it != tm.end() ) + { + // If the x coordinate is within "r" pixels of the node's position + if( posTicks >= it.key() - ticksOffset && posTicks <= it.key() + ticksOffset ) + { + // The y position of the node + float valueY = yCoordOfLevel(it.value().getInValue()); + // If the y coordinate is within "r" pixels of the node's value + if( y >= (valueY - r) && y <= (valueY + r) ) + { + node = it; + } + } + ++it; + } + + return node; +} + + + + From e5c96692ce96fc1918bb57ca1c452c70cba621dc Mon Sep 17 00:00:00 2001 From: IanCaio Date: Thu, 15 Oct 2020 14:01:40 -0300 Subject: [PATCH 04/61] Avoids unnecessary check in putValue When adding a value to a particular time, instead of checking if there's a node there already and manually setting its value, we just assign it with a new AutomationNode. QMap silently removes the existing node and adds the new one. --- src/core/AutomationPattern.cpp | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/core/AutomationPattern.cpp b/src/core/AutomationPattern.cpp index b313e8d94a0..c9826282395 100644 --- a/src/core/AutomationPattern.cpp +++ b/src/core/AutomationPattern.cpp @@ -215,17 +215,8 @@ MidiTime AutomationPattern::putValue( const MidiTime & time, Note::quantized( time, quantization() ) : time; - // If we do have a node for that midi time, set its value - if( m_timeMap.contains( newTime ) ) - { - m_timeMap[ newTime ].setInValue( value ); - m_timeMap[ newTime ].setOutValue( value ); - } - // If we don't have a node, create one - else - { - m_timeMap[ newTime ] = AutomationNode( value ); - } + // Create a node or replace the existing one on newTime + m_timeMap[ newTime ] = AutomationNode( value ); timeMap::iterator it = m_timeMap.find( newTime ); From ab0aeb08cb7f335cc1ce55474a5bb2d1e993926d Mon Sep 17 00:00:00 2001 From: IanCaio Date: Thu, 15 Oct 2020 20:40:26 -0300 Subject: [PATCH 05/61] Adds upgrade routine for the automation nodes Adds an upgrade method for the change in the automation nodes settings. The "value" attribute of the