New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some automation pattern fixes #3352

Merged
merged 9 commits into from Mar 4, 2017

Conversation

Projects
5 participants
@zonkmachine
Member

zonkmachine commented Feb 13, 2017

Fixes #391

  • In response to one part of issue #391, comment here, in which you can't delete Automation Points that were made under a finer quantization. Don't consider quantization when deleting and just delete the value at the position.
    We should maybe iterate through the neighbouring positions if there are no values at the current positon as it's a bit hard currently to hit the right spot for deletion. You have a similar problem when dragging an Automation Point which I haven't touched in this PR. When you select an Automation Point made under finer quantization you will instead create a new point at the quantized position.
  • Let an automation point clear the area it covers.
  • Triplets and no quantization option. This gives the Automation Editor the same quantization options as the other editors.
  • Improved erase action when moving mouse across Automation Points.
  • Eighth note, new default quantization value.
@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 13, 2017

Member

Found a better solution that keeps the quantization but defaults for minimal value.

Member

zonkmachine commented Feb 13, 2017

Found a better solution that keeps the quantization but defaults for minimal value.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Feb 13, 2017

Member

On an offtopic note, I think the automation editor should support all quantization values, it can't do triplets atm.

Member

Umcaruje commented Feb 13, 2017

On an offtopic note, I think the automation editor should support all quantization values, it can't do triplets atm.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 14, 2017

Member

it can't do triplets atm.

#3358

Member

zonkmachine commented Feb 14, 2017

it can't do triplets atm.

#3358

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 15, 2017

Member

OK. Reverted back to the first commit.

Member

zonkmachine commented Feb 15, 2017

OK. Reverted back to the first commit.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 15, 2017

Member

Tested something. Right now the quantized position only covers the starting point. Let it instead also cover the length of the Automation Point by just wiping clean the time positions it covers.

Member

zonkmachine commented Feb 15, 2017

Tested something. Right now the quantized position only covers the starting point. Let it instead also cover the length of the Automation Point by just wiping clean the time positions it covers.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 15, 2017

Member

The point to be deleted is determined on a pixel basis, so you may need to zoom in to get to all time values. With no quantization on AutomationPattern::removeValue() it gets overall slightly harder to delete an Automation Point but it depends on how you use the mouse. With the last fix it gets pretty easy. You just 'turn the quantization up' and write over the old values. Needs more testing.

Member

zonkmachine commented Feb 15, 2017

The point to be deleted is determined on a pixel basis, so you may need to zoom in to get to all time values. With no quantization on AutomationPattern::removeValue() it gets overall slightly harder to delete an Automation Point but it depends on how you use the mouse. With the last fix it gets pretty easy. You just 'turn the quantization up' and write over the old values. Needs more testing.

@zonkmachine zonkmachine changed the title from Fix deleting automation points out of quantization to Some automation pattern fixes Feb 20, 2017

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 20, 2017

Member

I bunched the three automation pattern/editor fixes together. Ready for testing

Member

zonkmachine commented Feb 20, 2017

I bunched the three automation pattern/editor fixes together. Ready for testing

@zonkmachine zonkmachine requested a review from tresf Feb 20, 2017

@zonkmachine zonkmachine added this to In Progress in Release 1.2.0 RC3 Feb 22, 2017

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 24, 2017

Member

@Umcaruje I fixed the algorithm for erase so it's smoother and catches Automation Points around it proportional to the zooming factor, but I don't know how it works on different resolutions. Can you test this and especially erasing Automation Points when right click + moving across them at various zoom levels?

Member

zonkmachine commented Feb 24, 2017

@Umcaruje I fixed the algorithm for erase so it's smoother and catches Automation Points around it proportional to the zooming factor, but I don't know how it works on different resolutions. Can you test this and especially erasing Automation Points when right click + moving across them at various zoom levels?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 25, 2017

Member

Merge?

Member

zonkmachine commented Feb 25, 2017

Merge?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 25, 2017

Member
  • Let an automation point clear the area it covers.

@Umcaruje Did you test this change? It gives the Automation Point the same length as it's quantization. I think it's a really smooth solution. You could make something like a shift key override for this but I don't think it would be used a whole lot.

Member

zonkmachine commented Feb 25, 2017

  • Let an automation point clear the area it covers.

@Umcaruje Did you test this change? It gives the Automation Point the same length as it's quantization. I think it's a really smooth solution. You could make something like a shift key override for this but I don't think it would be used a whole lot.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Feb 25, 2017

Member

Hmm, I'm not sure how much I like the deleting solution, now that I've tested it. It would be nice if it catches the 20-40px around the point, but not the whole length. It just surprises me from a UX perspective. I record automation sometimes and I delete unnecessary points, and while doing that I hold my right mouse key. With the new behaviour this just wipes clean along the Y axis, which feels weird.

Did you test this change? It gives the Automation Point the same length as it's quantization. I think it's a really smooth solution. You could make something like a shift key override for this but I don't think it would be used a whole lot.

Yes, it actually is something I would use rather than my previous method of erasing. I'd implement a shift override, because LMMS doesn't allow to have two automation points at the same position. This makes ramping automation hard to do, so I usually use the lowest quantisation to put the previous point as close to the next one. The new behaviour would make changing those points hard.

Member

Umcaruje commented Feb 25, 2017

Hmm, I'm not sure how much I like the deleting solution, now that I've tested it. It would be nice if it catches the 20-40px around the point, but not the whole length. It just surprises me from a UX perspective. I record automation sometimes and I delete unnecessary points, and while doing that I hold my right mouse key. With the new behaviour this just wipes clean along the Y axis, which feels weird.

Did you test this change? It gives the Automation Point the same length as it's quantization. I think it's a really smooth solution. You could make something like a shift key override for this but I don't think it would be used a whole lot.

Yes, it actually is something I would use rather than my previous method of erasing. I'd implement a shift override, because LMMS doesn't allow to have two automation points at the same position. This makes ramping automation hard to do, so I usually use the lowest quantisation to put the previous point as close to the next one. The new behaviour would make changing those points hard.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 25, 2017

Member

Hmm, I'm not sure how much I like the deleting solution, now that I've tested it. It would be nice if it catches the 20-40px around the point, but not the whole length.

Is this about erasing when sweeping with the mouse? For me the automation point is deleted when the mouse is at about 'one automation point diameter' distance. What is your screen resolution?

Yes, it actually is something I would use rather than my previous method of erasing. I'd implement a shift override, because LMMS doesn't allow to have two automation points at the same position.

OK. Will try the shift fix..

Member

zonkmachine commented Feb 25, 2017

Hmm, I'm not sure how much I like the deleting solution, now that I've tested it. It would be nice if it catches the 20-40px around the point, but not the whole length.

Is this about erasing when sweeping with the mouse? For me the automation point is deleted when the mouse is at about 'one automation point diameter' distance. What is your screen resolution?

Yes, it actually is something I would use rather than my previous method of erasing. I'd implement a shift override, because LMMS doesn't allow to have two automation points at the same position.

OK. Will try the shift fix..

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 26, 2017

Member

It would be nice if it catches the 20-40px around the point,

Right, I think I got you. Some space to counter this #3301 ?
I've tried to deal with it but I pretty much failed there unfortunately. I'll maybe look at it again but not right now. I'm taking the 'automation point length' hack out for now and send it back to the drawing board.

Member

zonkmachine commented Feb 26, 2017

It would be nice if it catches the 20-40px around the point,

Right, I think I got you. Some space to counter this #3301 ?
I've tried to deal with it but I pretty much failed there unfortunately. I'll maybe look at it again but not right now. I'm taking the 'automation point length' hack out for now and send it back to the drawing board.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Feb 28, 2017

Member

You could make something like a shift key override for this but I don't think it would be used a whole lot.

Yes, it actually is something I would use rather than my previous method of erasing. I'd implement a shift override, because LMMS doesn't allow to have two automation points at the same position. This makes ramping automation hard to do, so I usually use the lowest quantisation to put the previous point as close to the next one. The new behaviour would make changing those points hard.

OK. Shift key was busy so I used the Ctrl key.

Member

zonkmachine commented Feb 28, 2017

You could make something like a shift key override for this but I don't think it would be used a whole lot.

Yes, it actually is something I would use rather than my previous method of erasing. I'd implement a shift override, because LMMS doesn't allow to have two automation points at the same position. This makes ramping automation hard to do, so I usually use the lowest quantisation to put the previous point as close to the next one. The new behaviour would make changing those points hard.

OK. Shift key was busy so I used the Ctrl key.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 2, 2017

Member

I think this works well.
@Umcaruje Can you test this and see if the <Ctrl> button works to override the quantization length function?

@tresf I think this closes #391 though there is more than one issue in that ticket but it looks like the export bug part is a bit dated.

Member

zonkmachine commented Mar 2, 2017

I think this works well.
@Umcaruje Can you test this and see if the <Ctrl> button works to override the quantization length function?

@tresf I think this closes #391 though there is more than one issue in that ticket but it looks like the export bug part is a bit dated.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Mar 2, 2017

Member

@zonkmachine I don't have time to build and test this, so if someone else can confirm that automation points smaller than default Quantization can be edited/deleted with this PR, the bug should be closed.

the export bug part is a bit dated.

I had originally assumed they were related, but on second glance it should be reopened as a separate item if still valid after this is closed.

Member

tresf commented Mar 2, 2017

@zonkmachine I don't have time to build and test this, so if someone else can confirm that automation points smaller than default Quantization can be edited/deleted with this PR, the bug should be closed.

the export bug part is a bit dated.

I had originally assumed they were related, but on second glance it should be reopened as a separate item if still valid after this is closed.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 2, 2017

Member

I had originally assumed they were related, but on second glance it should be reopened as a separate item if still valid after this is closed.

I had a second look at this and it is of course the same issue. Multiples of 3 is the positions for 1/64 quantization so if you erase with 1/16 quantization you'll get those values. Just duplicated the bug.

dots

Member

zonkmachine commented Mar 2, 2017

I had originally assumed they were related, but on second glance it should be reopened as a separate item if still valid after this is closed.

I had a second look at this and it is of course the same issue. Multiples of 3 is the positions for 1/64 quantization so if you erase with 1/16 quantization you'll get those values. Just duplicated the bug.

dots

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 2, 2017

Member

But then again you have the issue of the volume automation and that's not really related...

Member

zonkmachine commented Mar 2, 2017

But then again you have the issue of the volume automation and that's not really related...

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 4, 2017

Member

If there are no further comments I'll merge this later this evening.

Member

zonkmachine commented Mar 4, 2017

If there are no further comments I'll merge this later this evening.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Mar 4, 2017

Member

So I did further testing, the Ctrl override works neatly, but I think we should display a Hint so users actually know that it exists.

The deleting behaviour is not something that I really like, cause it will make accidental deletes much more common imo. Example:
gifrecord_2017-03-04_110711

Member

Umcaruje commented Mar 4, 2017

So I did further testing, the Ctrl override works neatly, but I think we should display a Hint so users actually know that it exists.

The deleting behaviour is not something that I really like, cause it will make accidental deletes much more common imo. Example:
gifrecord_2017-03-04_110711

@nihilazo

This comment has been minimized.

Show comment
Hide comment
@nihilazo

nihilazo Mar 4, 2017

all seems working to me, I agree with @Umcaruje there should be a hint for the ctrl override.

nihilazo commented Mar 4, 2017

all seems working to me, I agree with @Umcaruje there should be a hint for the ctrl override.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 4, 2017

Member

whatsup

Qt cuts off the last words sometimes?

	m_quantizeComboBox->setWhatsThis( tr( "Quantization. Sets the smallest "
				"step size for the Automation Point. By default "
				"this also sets the length of the Automation "
				"Point, clearing out other points in the range. "
				"Press <Ctrl> to override this behaviour." ) );
Member

zonkmachine commented Mar 4, 2017

whatsup

Qt cuts off the last words sometimes?

	m_quantizeComboBox->setWhatsThis( tr( "Quantization. Sets the smallest "
				"step size for the Automation Point. By default "
				"this also sets the length of the Automation "
				"Point, clearing out other points in the range. "
				"Press <Ctrl> to override this behaviour." ) );
@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 4, 2017

Member

The deleting behaviour is not something that I really like, cause it will make accidental deletes much more common imo. Example:

Dropping the quantization on delete makes it more or less a necessity. It's painful enough in 1.1.3 .
Should I make the action a bit closer to the point to delete?

Member

zonkmachine commented Mar 4, 2017

The deleting behaviour is not something that I really like, cause it will make accidental deletes much more common imo. Example:

Dropping the quantization on delete makes it more or less a necessity. It's painful enough in 1.1.3 .
Should I make the action a bit closer to the point to delete?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 4, 2017

Member

@lukeoftheaura @Umcaruje Thanks for testing. I made the erase/delete a bit less aggressive and the whatsthis now reads:

setWhatsThis( tr( "Quantization. Sets the smallest "
				"step size for the Automation Point. By default "
				"this also sets the length, clearing out other "
				"points in the range. Press <Ctrl> to override "
				"this behaviour." ) );
Member

zonkmachine commented Mar 4, 2017

@lukeoftheaura @Umcaruje Thanks for testing. I made the erase/delete a bit less aggressive and the whatsthis now reads:

setWhatsThis( tr( "Quantization. Sets the smallest "
				"step size for the Automation Point. By default "
				"this also sets the length, clearing out other "
				"points in the range. Press <Ctrl> to override "
				"this behaviour." ) );
@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 4, 2017

Member

I'm not sure what's wrong in the animation though. It looks pretty good and I see no accidental deletions in it.

ps. I'm still on schedule to merge this later on... 😃

Member

zonkmachine commented Mar 4, 2017

I'm not sure what's wrong in the animation though. It looks pretty good and I see no accidental deletions in it.

ps. I'm still on schedule to merge this later on... 😃

@zonkmachine zonkmachine merged commit f7d09c3 into LMMS:master Mar 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zonkmachine zonkmachine deleted the zonkmachine:automationpatterndelete branch Mar 4, 2017

@zonkmachine zonkmachine moved this from In Progress to Done in Release 1.2.0 RC3 Mar 4, 2017

PaulBatchelor added a commit to PaulBatchelor/lmms that referenced this pull request Mar 5, 2017

Some automation pattern fixes (#3352)
* Fix deleting automation points out of quantization

* Triplets in Automation Editor + better remove action

* Let a quantized Automation point wipe clean the space it covers

* Improve sensitivity on erase with zoom < 100%

* Eigth note default quantization

* Tooltip and whatsthis text
MidiTime putValue( const MidiTime & time,
const float value,
const bool quantPos = true,
const bool controlKey = false );

This comment has been minimized.

@jasp00

jasp00 Mar 17, 2017

Member

Core objects should not have references to the control key, which is a UI concept.

@jasp00

jasp00 Mar 17, 2017

Member

Core objects should not have references to the control key, which is a UI concept.

This comment has been minimized.

@zonkmachine

zonkmachine Mar 22, 2017

Member

Ouch! I'm looking into this now but I'm not sure how to fix that. The objective is to be able to turn off the feature to give the control point a 'minimum length' according to it's quantization value.

@zonkmachine

zonkmachine Mar 22, 2017

Member

Ouch! I'm looking into this now but I'm not sure how to fix that. The objective is to be able to turn off the feature to give the control point a 'minimum length' according to it's quantization value.

This comment has been minimized.

@jasp00

jasp00 Mar 22, 2017

Member

You have two choices:

  1. The clean way: Move the feature to the view object, which reacts to the control key.
  2. The easy way: Keep the feature in the model and rename controlKey to something like ignoreSurroundingPoints.
@jasp00

jasp00 Mar 22, 2017

Member

You have two choices:

  1. The clean way: Move the feature to the view object, which reacts to the control key.
  2. The easy way: Keep the feature in the model and rename controlKey to something like ignoreSurroundingPoints.

This comment has been minimized.

@zonkmachine

zonkmachine Mar 23, 2017

Member
  1. The easy way: Keep the feature in the model and rename controlKey to something like ignoreSurroundingPoints.

Do you mean like:

diff --git a/include/AutomationPattern.h b/include/AutomationPattern.h
index 480b69f..1bb5c03 100644
--- a/include/AutomationPattern.h
+++ b/include/AutomationPattern.h
@@ -79,7 +79,7 @@ public:
        MidiTime putValue( const MidiTime & time,
                                const float value,
                                const bool quantPos = true,
-                               const bool controlKey = false );
+                               const bool ignoreSurroundingPoints = false );
 
        void removeValue( const MidiTime & time );
 
diff --git a/src/core/AutomationPattern.cpp b/src/core/AutomationPattern.cpp
index ffe13ff..7996af3 100644
--- a/src/core/AutomationPattern.cpp
+++ b/src/core/AutomationPattern.cpp
@@ -208,7 +208,7 @@ void AutomationPattern::updateLength()
 MidiTime AutomationPattern::putValue( const MidiTime & time,
                                        const float value,
                                        const bool quantPos,
-                                       const bool controlKey )
+                                       const bool ignoreSurroundingPoints )
 {
        cleanObjects();
 
@@ -221,7 +221,7 @@ MidiTime AutomationPattern::putValue( const MidiTime & time,
 
        // Remove control points that are covered by the new points
        // quantization value. Control Key to override
-       if( ! controlKey )
+       if( ! ignoreSurroundingPoints )
        {
                for( int i = newTime + 1; i < newTime + quantization(); ++i )
                {
@zonkmachine

zonkmachine Mar 23, 2017

Member
  1. The easy way: Keep the feature in the model and rename controlKey to something like ignoreSurroundingPoints.

Do you mean like:

diff --git a/include/AutomationPattern.h b/include/AutomationPattern.h
index 480b69f..1bb5c03 100644
--- a/include/AutomationPattern.h
+++ b/include/AutomationPattern.h
@@ -79,7 +79,7 @@ public:
        MidiTime putValue( const MidiTime & time,
                                const float value,
                                const bool quantPos = true,
-                               const bool controlKey = false );
+                               const bool ignoreSurroundingPoints = false );
 
        void removeValue( const MidiTime & time );
 
diff --git a/src/core/AutomationPattern.cpp b/src/core/AutomationPattern.cpp
index ffe13ff..7996af3 100644
--- a/src/core/AutomationPattern.cpp
+++ b/src/core/AutomationPattern.cpp
@@ -208,7 +208,7 @@ void AutomationPattern::updateLength()
 MidiTime AutomationPattern::putValue( const MidiTime & time,
                                        const float value,
                                        const bool quantPos,
-                                       const bool controlKey )
+                                       const bool ignoreSurroundingPoints )
 {
        cleanObjects();
 
@@ -221,7 +221,7 @@ MidiTime AutomationPattern::putValue( const MidiTime & time,
 
        // Remove control points that are covered by the new points
        // quantization value. Control Key to override
-       if( ! controlKey )
+       if( ! ignoreSurroundingPoints )
        {
                for( int i = newTime + 1; i < newTime + quantization(); ++i )
                {

This comment has been minimized.

@jasp00

jasp00 Mar 28, 2017

Member

Yes.

@jasp00

This comment has been minimized.

@zonkmachine

zonkmachine Mar 28, 2017

Member

Thanks! Fixed here: #3459

@zonkmachine

zonkmachine Mar 28, 2017

Member

Thanks! Fixed here: #3459

PhysSong added a commit that referenced this pull request Nov 19, 2017

Fix automation pattern regressions (#3977)
Change the default value of ignoreSurroundingPoints in AutomationPattern::putValue to true, which was false in #3352.
Fixes automation filpping bug and some potential issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment