Adds automation flipping. #1486

Spekular · 2014-12-22T22:42:17Z

Implements #1289.

Spekular · 2014-12-22T22:48:16Z

@diizy flipping horizontally in song editor calls flip with the boolean "visible" set to true, doing so from automaton editor does so with it set to false. Once I figure out how to flip it based on pattern length in song editor (which I might need some help with), I can set it up so it behaves differently based on which editor it's used from.

tresf · 2014-12-23T02:19:24Z

src/core/AutomationPattern.cpp

+	for( int i = 0; i <= numPoints; i++ )
+	{
+
+		if (min < 0)


formatting :) if ( min < 0 )

tresf · 2014-12-23T02:22:56Z

Please make the formatting changes (there are a few more I left out). You can push to this same branch and it will automatically be reflected in the pull request.

Spekular · 2014-12-23T08:25:18Z

Thanks! I'll fix it asap

Spekular · 2014-12-23T09:17:07Z

While editing AutomationPatternView I saw m_pat->length(), which seems like it would give the length of the pattern in song editor. I have a couple different ideas on how to proceed now.
Option 1:
Pass a bool to flipX, try to get pattern's song editor length in AutomationPattern
Option 2:
Pass a bool plus m_pat->length() to flipX, use the passed length only when bool is true.
Option 3:
Pass nothing when flipping in auto editor, pass length when flipping in song editor. Create flipX() and flipX( length )

Option 1 might not be possible, Option 2 bothers me (I shouldn't have to pass two variables when I can pass 0/1 instead with option 3), and Option 3 creates two entire separate functions, which might be overkill. @diizy which do you think is best?

diizy · 2014-12-23T09:42:55Z

On 12/23/2014 11:17 AM, Spekular wrote:

While editing AutomationPatternView I saw m_pat->length(), which seems
like it would give the length of the pattern in song editor. I have a
couple different ideas on how to proceed now.
Option 1:
Pass a bool to flipX, try to get pattern's song editor length in
AutomationPattern
Option 2:
Pass a bool plus m_pat->length() to flipX, use the passed length only
when bool is true.
Option 3:
Pass nothing when flipping in auto editor, pass length when flipping
in song editor. Create flipX() and flipX( length )

Option 1 might not be possible, Option 2 bothers me (I shouldn't have
to pass two variables when I can pass 0/1 instead with option 3), and
Option 3 creates two entire separate functions, which might be
overkill. @diizy https://github.com/diizy which do you think is best?

How about you create a function with a default parameter: flipX( int
length = -1 )

So if you call flipX() it's the same as calling flipX(-1)

Then in the function just check the length parameter, if it's negative,
act as if there are no args, if it's positive, use it as function parameter.

Spekular · 2014-12-23T09:53:23Z

@diizy I wasn't aware you could do that, thanks EDIT: I've cleaned up the code a bit by removing commented out lines and making it adhere better to standards.

diizy · 2014-12-23T13:00:28Z

On 12/23/2014 02:04 PM, Spekular wrote:

@diizy https://github.com/diizy I'm having issues with this. I've
updated the function to |void AutomationPattern::flipX( int length =
-1 )|, but it's not working correctly. I enclose the entire flipping
operation in |if (length = -1)| as a test, but the flip still occurs
when triggered from song editor. (It shouldn't, since length is passed
from AutomationPatternView and should != -1)

Upload the code and I'll take a look

tresf · 2014-12-23T13:24:39Z

@diizy I wasn't aware you could do that, thanks!

Me neither. I've done this in other looser typed languages, but never in C++. Good to know. 👍

-Tres

Spekular · 2014-12-23T13:40:11Z

@diizy That was my fault, it wasn't building the changes because I was modifying my backup and not the actual code. Anyways, progress is pretty good.
Here's a pattern before flipping:

Here it is after flipping in the automation editor:

Here I've flipped it from the song editor with the pattern dragged to four bars: (a little broken)

(Note there's a slight inconsistency that I'm going to fix by deleting the point at position 0 after flipping, it's added at the end of the visible pattern and then the resulting pattern gets flipped)
Here it is after flipping in the song editor with only 1 bar visible, obviously still broken:

Spekular · 2014-12-23T14:03:10Z

@diizy @tresf ok so m_pat->length() is still the wrong length. I changed the code to add a point at m_pat->() length, and no matter how large I scaled the pattern in song editor it was 3 bars (I guess the point right on the end of bar two counts as the beginning of bar three and it rounds up). Do any of you know how to actually get the visibile length of an automation pattern in the song editor? It has to be stored somewhere and it's the one thing holding this up.

tresf · 2014-12-23T14:27:27Z

No, I don't but finding out how other parts of the software do it is your best best. Perhaps somewhere in MidiTime.h.

The idea is that each Tact represents a certain amount of ticks. Perhaps that can help? Sorry I'm not of more use.

Spekular · 2014-12-23T16:58:03Z

I've got a not very good solution. width() in AutomationPatternView gives a pixel width. Then I divide that by pixelsPerTact and multiply by tickPerTact to get the number of ticks. The issue with this is that it's not precise enough, but I don't see any actual measurement of ticks. Rounding to the nearest bar would fix this when that's desired, but still wouldn't allow precise dragging/flipping.

diizy · 2014-12-23T17:06:24Z

On 12/23/2014 04:03 PM, Spekular wrote:

@diizy https://github.com/diizy @ tresf ok so m_pat->length() is
still the wrong length. I changed the code to add a point at m_pat->()
length, and no matter how large I scaled the pattern in song editor it
was 3 bars (I guess the point right on the end of bar two counts as
the beginning of bar three and it rounds up). Do any of you know how
to actually get the visibile length of an automation pattern in the
song editor? It has to be stored somewhere and it's the one thing
holding this up.

Try TrackContentObject::length()

as in

m_pat->TrackContentObject::length()

tresf · 2014-12-23T17:10:10Z

Yeah as Vesa said, length is of type MidiTime already, you can treat it as such.

I believe the confusing part is that you can treat length() as a number and use its underlying value, but don't. Treat it like a MidiTime object. Pattern.h makes this pretty clear as length() always returns a MidiTime object.

tresf · 2014-12-23T17:11:15Z

Sorry smartphone makes that close button too easy to click on :)

diizy · 2014-12-23T17:14:43Z

On 12/23/2014 07:10 PM, Tres Finocchiaro wrote:

Yeah as Vesa said, length is if type MidiTime already, you can treat
it as such.

I beliece the confusing part is that you can treat length() as a
number and use it's underlying value, but don't. Treat it like a
MidiTime object. Pattern.h makes this pretty clear

That doesn't actually matter, MidiTime converts automatically to int
(there's a cast overload). The issue is, AutomationPattern::length()
gives a different value than TrackContentObject::length(), even though
the objects are relatives to each other. It's a bit stupid but hey...
anyway, the TCO one should give the length based on the visible pattern
length.

diizy · 2014-12-23T17:15:19Z

On 12/23/2014 06:58 PM, Spekular wrote:

I've got a not very good solution. width() in AutomationPatternView
gives a pixel width. Then I divide that by pixelsPerTact and multiply
by tickPerTact to get the number of ticks. The issue with this is that
it's not precise enough, but I don't see any actual measurement of
ticks. Rounding to the nearest bar would fix this when that's desired,
but still wouldn't allow precise dragging/flipping.

Don't do that. Just use TrackContentObject::length()

tresf · 2014-12-23T17:45:02Z

the TCO one should give the length based on the visible pattern
length.

👍

Spekular · 2014-12-23T18:34:53Z

Ok, so flipping horizontally and vertically both work in automation editor, flipping the pattern only. Flipping horizontally in song editor flips the visible area properly no matter if the pattern is larger, smaller than, or the same size as the visible area. Flipping vertically from song editor does not currently take into account the visible area, but this should be a relatively easy fix (I'd forgotten about it, which is why I haven't gotten it done).

diizy · 2014-12-24T18:29:25Z

Flipping vertically can probably stay as it is...

diizy · 2014-12-24T18:34:31Z

I'll probably need to draw the icons to make sure they fit in with the other icons... other than that, things look ok here.

Adds automation flipping.

Spekular · 2014-12-25T10:33:39Z

:D Awesome!

tresf · 2014-12-25T18:17:20Z

Congrats @Spekular. Thanks for the hard work!

I think we already have some compelling reasons to start building betas of 1.2. Next milestone... Here we come :)

If we could give codenames, I think 1.0: Codename "Fundamental". 1.1 Codename "Instrumental". 1.2 Codename: "Precision" :)

curlymorphic · 2014-12-25T21:03:42Z

Codename: "Precision"

:)

diizy · 2014-12-30T18:32:50Z

src/core/AutomationPattern.cpp

+			for( int i = 0; i <= numPoints; i++ )
+			{
+				tempValue = valueAt( ( iterate + i ).key() );
+				cleanObjects();


what is the purpose of the cleanObjects() call here? Flipping shouldn't require any changes to the automation connections.

@vesa I'm not entirely sure, I don't recall adding it.

On 12/30/2014 09:02 PM, Spekular wrote:

In src/core/AutomationPattern.cpp
#1486 (diff):

numPoints++;

}

float realLength = ( iterate + numPoints ).key();

if ( length != -1 && length != realLength)

{

if ( realLength < length )

{

tempValue = valueAt( ( iterate + numPoints ).key() );

putValue( MidiTime( length ) , tempValue, false);

numPoints++;

for( int i = 0; i <= numPoints; i++ )

{

tempValue = valueAt( ( iterate + i ).key() );

cleanObjects();
@vesa https://github.com/Vesa I'm not entirely sure, I don't recall
adding it.

Then it should be removed.

Perhaps this commit: https://github.com/Spekular/Automation-Flip/commit/ddbf9cd26448ac4b779cf39aaeaf6c9ac650a52b

@vesa will do ASAP.

Spekular · 2014-12-31T17:06:52Z

@vesa Should be fixed now!

Spekular · 2015-01-05T14:11:29Z

@diizy is it safe to delete my branch for this now?

diizy · 2015-01-05T14:12:26Z

Sorry, where is the commit for the fixes?

Spekular · 2015-01-05T14:12:53Z

@diizy it should be the latest one. Spekular@d533bce

tresf · 2015-01-05T14:15:09Z

@diizy is it safe to delete my branch for this now?

Well... you called your branch "master" and your changes have already been merged. Do you want to delete your own master branch? :)

Normally in your case a developer would fast forward his/her master branch and continue using it, although ideally these types of changes would be on a separate branch that could be easily deleted once merged.

diizy · 2015-01-05T14:17:10Z

On 01/05/2015 04:12 PM, Spekular wrote:

@diizy https://github.com/diizy it should be the latest one.

You can't add commits to a pr after it's merged. You need to do a new pr.

Spekular · 2015-01-05T14:17:23Z

@tresf yeah it's a bit of a mess, which is why I wanna delete the entire fork and make a new proper one. The new commits don't seem to be showing up as merge-able or merged, so I suppose I should make a new pr? EDIT: @diizy ok

tresf · 2015-01-05T14:20:16Z

@tresf yeah it's a bit of a mess, which is why I wanna delete the entire fork and make a new proper one. The new commits don't seem to be showing up as merge-able or merged, so I suppose I should make a new pr?

Well, yes... sort of. First, backup your code changes. Next, fast forward per our wiki tutorial. Last, verify your code is good and make a new PR.

If you really want to clean house, backup your code, hard reset your master branch, and do future changes via a dedicated branch as to not mess up your own master.

Lastly, you can always delete your repo on GitHub which will really clean things up... but if you're like me and use it for other things (namely Windows binary releases and Wiki articles), you may want to weigh your options first due to the amount of work involved in carrying the content over 👍 .

tresf · 2015-01-05T15:14:58Z

Related #1554

tresf reviewed Dec 23, 2014
View reviewed changes

src/core/AutomationPattern.cpp

for( int i = 0; i <= numPoints; i++ )

{

if (min < 0)

Copy link

Member

tresf Dec 23, 2014

Choose a reason for hiding this comment

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

formatting :) if ( min < 0 )

tresf closed this Dec 23, 2014

tresf reopened this Dec 23, 2014

Spekular added 3 commits December 23, 2014 19:29

Adds automation flipping.

eb0b0f5

Code Cleanup

bfeced7

Fixes flipping visiblesegment horizontally

ddbf9cd

diizy added a commit that referenced this pull request Dec 24, 2014

Merge pull request #1486 from Spekular/master

9e4db14

Adds automation flipping.

diizy merged commit 9e4db14 into LMMS:master Dec 24, 2014

diizy reviewed Dec 30, 2014
View reviewed changes

tresf mentioned this pull request Jan 5, 2015

enhancement: automation, add option 'Mirror pattern' #1289

Closed

Adds automation flipping. #1486

Adds automation flipping. #1486

Conversation

Spekular commented Dec 22, 2014

Spekular commented Dec 22, 2014

tresf Dec 23, 2014

Choose a reason for hiding this comment

tresf commented Dec 23, 2014

Spekular commented Dec 23, 2014

Spekular commented Dec 23, 2014

diizy commented Dec 23, 2014

Spekular commented Dec 23, 2014

diizy commented Dec 23, 2014

tresf commented Dec 23, 2014

Spekular commented Dec 23, 2014

Spekular commented Dec 23, 2014

tresf commented Dec 23, 2014

Spekular commented Dec 23, 2014

diizy commented Dec 23, 2014

tresf commented Dec 23, 2014

tresf commented Dec 23, 2014

diizy commented Dec 23, 2014

diizy commented Dec 23, 2014

tresf commented Dec 23, 2014

Spekular commented Dec 23, 2014

diizy commented Dec 24, 2014

diizy commented Dec 24, 2014

Spekular commented Dec 25, 2014

tresf commented Dec 25, 2014

curlymorphic commented Dec 25, 2014

diizy Dec 30, 2014

Choose a reason for hiding this comment

Spekular Dec 30, 2014

Choose a reason for hiding this comment

diizy Dec 30, 2014

Choose a reason for hiding this comment

tresf Dec 30, 2014

Choose a reason for hiding this comment

Spekular Dec 30, 2014

Choose a reason for hiding this comment

Spekular commented Dec 31, 2014

Spekular commented Jan 5, 2015

diizy commented Jan 5, 2015

Spekular commented Jan 5, 2015

tresf commented Jan 5, 2015

diizy commented Jan 5, 2015

Spekular commented Jan 5, 2015

tresf commented Jan 5, 2015

tresf commented Jan 5, 2015