Beat note bug at the end of bars #2964

zonkmachine · 2016-08-08T19:17:04Z

I had a go at fixing the problem with beat notes on position near the end of the beat forcing the insert of an extra bar. ~~I also tweaked the Piano Roll notes to not do the same depending on where they end but on the start position. To me that's more logical behaviour.~~

Edit2: This is now a pure fix for #1105 and the rest is edited out and will be a separate PR.

zonkmachine · 2016-08-08T19:37:26Z

I also tweaked the Piano Roll notes to not do the same depending on where they end but on the start position. To me that's more logical behaviour.

I mean more logical for notes in a BBTrack. Just an experiment...

zonkmachine · 2016-08-08T20:23:42Z

It's really a bit tricky this one with BBTrack vs. InstrumentTracks. Maybe, instead of checking the pattern type , it would be better to let the deciding factor of how the pattern is played back be the track type. If it's played from within a BBTrack it loops like this PR and if it's played from an instrument track the beat notes loops like the first commit while ordinary notes creates a wider space if they go over the the beat?

Umcaruje · 2016-08-11T23:27:25Z

👍 I really like this solution. Feels really natural and also fixes the original problem. I was fearing a note extending to the next bar that gets cut off at the end of a loop wouldn't ever get a noteoff, but this is not the case, the note plays out for its length and works perfectly, tested with zyn and VST's too.

zonkmachine · 2016-08-25T23:03:12Z

I was fearing a note extending to the next bar that gets cut off at the end of a loop wouldn't ever get a noteoff,

And I was fearing it would just add to the volume and get noisy... and weeellll it can do. I've mostly sorted the backward compatibility but will split this into two parts. The bug fix separate and then the change in behaviour which needs a discussion.

zonkmachine · 2016-08-30T08:36:16Z

Here is the fix for #1105
The backward compattbility part in Pattern::loadSettings() is longer than usually but pretty straight forward. We just check for version and then, if it's a beat pattern, load it in the old way. The final check is for patterns that have been manually adjusted. Works but needs more testing.

Umcaruje · 2016-08-30T15:02:31Z

I think the backwards support should be handled in Datafile::Upgrade, if possible

zonkmachine · 2016-08-30T15:37:20Z

I'll look into Datafile::Upgrade, and also I think I've found a way to make it not so ridiculously long...

jasp00 · 2016-08-30T15:46:25Z

I will have a look at #2969, it should be fixed first for the upgrade.

zonkmachine · 2016-09-08T18:58:53Z

I suggest to postpone this PR to 1.3 .

jasp00 · 2016-09-09T00:19:48Z

I suggest to postpone this PR to 1.3 .

Why? The bug is important. The only problem with this request is the need for an upgrade function. Let us merge #3011 (which fixes #2969) and finish this one.

zonkmachine · 2016-09-09T00:45:20Z

OK. I'll look into it again.

zonkmachine · 2016-12-22T23:31:53Z

The only problem with this request is the need for an upgrade function.

Looking at it now. It seems like there is more than an upgrade function needed here. 🪲

zonkmachine · 2016-12-23T11:24:36Z

Right. In #3037 setting "len" was dropped and this holds the length of the beat so I have to read in "len" and convert it to "steps" directly.
Edit: Note to self, local branch name: beatnotenew

zonkmachine · 2017-01-17T21:47:44Z

#2030 / #3105 needs to be fixed before an upgrade path can be fully tested for this. Not a big problem, just a note.

zonkmachine · 2017-01-17T22:39:32Z

@jasp00 ^Since we don't know for sure what the time signature will be, as this could either be changed manually or automated, I don't think I can do the upgrade in DataFile.cpp as I'm working on now.
I think it should be done in Pattern::changeTimeSignature() and a call to Pattern::changeTimeSignature() should perhaps be added to Pattern::loadSettings() to take care of the initial state. Does this sound about right to you?

jasp00 · 2017-01-20T17:56:09Z

There is a design problem: it does not make sense to update patterns with time signatures on playback. If you change the signature, you break the pattern; each pattern should have its own signature. changeTimeSignature() is being used as a hack right now. So it does not really matter what you do now, you may proceed as you see fit.

zapashcanon · 2017-01-21T15:42:10Z

src/tracks/Pattern.cpp

+			MidiTime oldLength = MidiTime( _this.attribute( "len" ).toInt() );
+
+			tick_t max_length = MidiTime::ticksPerTact();
+			int oldSteps = m_steps;


const int oldSteps = m_steps;

Thanks, but if you follow the thread you will see that this is old code that's going to change completely. Also, I use the tag In progress for stuff that isn't really ready for review.

zonkmachine · 2017-02-03T15:22:19Z

@jasp00 I haven't given up on the DataFile.cpp upgrade path for this one. I'm just reverting to the original commit for now.

zonkmachine · 2017-02-03T16:52:03Z

Oh brother!
The algorithm is: "steps" = "len" / 12
It's that simple... 🚜

zonkmachine · 2017-02-03T17:21:09Z

src/core/DataFile.cpp

@@ -898,6 +899,25 @@ void DataFile::upgrade_1_1_91()
 			el.setAttribute( "arpdir", "3" );
 		}
 	}
+


This should be in it's own upgrade function, right?
void DataFile::upgrade_1_1_91()
What's the new upgrade function called _1_1_92() ? _1_2_0_rc3 ?

_1_2_0_rc3

Yes, 1.2.0-rc3 will be the next release assuming the fix is that specific. If it's only a regression since 1.1.x, it's preferred to keep it in the existing 1.1.91 section.

zonkmachine · 2017-02-03T20:50:31Z

@tresf I think I'm done here. The bug in question, actually I think it's a feature that just didn't cut it, goes way back. Before 1.0 .

zonkmachine · 2017-02-04T11:28:48Z

Merge?

tresf · 2017-02-04T15:03:17Z

src/core/DataFile.cpp

+			QDomElement el = list.item( i ).toElement();
+			for( int i = 0; !list.item( i ).isNull(); ++i )
+			{
+				patternLength = el.attribute( "len" ).toInt();


Do all "pattern" attributes contain an integer value (never blank?). We just need to make sure we sanitize our input. I think this change is fine, but any time we assume a String is an integer we need to make sure we don't segfault on something simple.

Nice catch. I'll test it some more with:

if( el.attribute( "len" ) != "" ) { patternLength = el.attribute( "len" ).toInt(); steps = patternLength / 12; el.setAttribute( "steps", steps ); }

Pushed a fix. With or without this the projects open up just like before. If I create some len="" in a .mmp the results are a bit buggy so I think that indicates there probably are no blank "len".
But that doesn't compare to testing real project files and they come out just fine. I don't know how to devise a good test case for this.

If I create some len="" in a .mmp the results are a bit buggy so I think that indicates there probably are no blank "len".

That was my suspicion as well but I wanted to mention it as I'm not sure how they're stored for all use-cases.

OK. I have no more tests do do here but as it changes the behaviour of a central tool maybe leave it up for a while before merge. Is it too optimistic to think that someone is going to bother to take it for a spin? We have quite a list of pull requests to go through. :)

zonkmachine · 2017-02-08T21:30:44Z

I'm going to wait with merging this for the outcome of the discussion on upgrade methods here.

tresf

Please don't wait. The comments in that bug report have to do with automating the versioning. Your logic is solid regardless of the outcome of that discussion.

tresf · 2017-02-08T21:51:43Z

src/core/DataFile.cpp

@@ -901,6 +901,32 @@ void DataFile::upgrade_1_1_91()
 }


+void DataFile::upgrade_1_2_0_rc3()
+{
+	// Upgrad from earlier bbtrack beat note behaviour of adding


typo, Upgrade

zonkmachine · 2017-02-09T01:46:54Z

OK. Merge...

zonkmachine mentioned this pull request Aug 25, 2016

Request change in model when dropping a track #3000

Merged

zonkmachine force-pushed the beatTrackAndPianoRollFixes branch from a7e8aa1 to edca72b Compare August 30, 2016 08:22

zonkmachine changed the title ~~Beat track and piano roll fixes~~ Beat note bug at the end of bars Aug 30, 2016

zonkmachine mentioned this pull request Sep 5, 2016

Fix sample track view in BB editor #3002

Merged

zonkmachine added this to the 1.3.0 milestone Sep 8, 2016

zonkmachine added the in progress label Dec 23, 2016

zapashcanon suggested changes Jan 21, 2017

View reviewed changes

zonkmachine force-pushed the beatTrackAndPianoRollFixes branch from edca72b to 188cdac Compare February 3, 2017 15:12

zonkmachine force-pushed the beatTrackAndPianoRollFixes branch from 188cdac to 7a7587c Compare February 3, 2017 17:14

zonkmachine commented Feb 3, 2017

View reviewed changes

zonkmachine force-pushed the beatTrackAndPianoRollFixes branch from a3d493b to 62c8700 Compare February 3, 2017 20:41

zonkmachine modified the milestones: 1.2.0, 1.3.0 Feb 3, 2017

zonkmachine removed the in progress label Feb 3, 2017

tresf reviewed Feb 4, 2017

View reviewed changes

zonkmachine force-pushed the beatTrackAndPianoRollFixes branch from f410d05 to 70250a2 Compare February 8, 2017 21:16

tresf reviewed Feb 8, 2017

View reviewed changes

No extra bar with beat note near end

9fe2dd7

zonkmachine force-pushed the beatTrackAndPianoRollFixes branch from 70250a2 to 9fe2dd7 Compare February 8, 2017 22:13

zonkmachine merged commit f367604 into LMMS:master Feb 9, 2017

zonkmachine deleted the beatTrackAndPianoRollFixes branch February 9, 2017 02:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beat note bug at the end of bars #2964

Beat note bug at the end of bars #2964

zonkmachine commented Aug 8, 2016 •

edited

Loading

zonkmachine commented Aug 8, 2016

zonkmachine commented Aug 8, 2016

Umcaruje commented Aug 11, 2016

zonkmachine commented Aug 25, 2016

zonkmachine commented Aug 30, 2016

Umcaruje commented Aug 30, 2016

zonkmachine commented Aug 30, 2016

jasp00 commented Aug 30, 2016

zonkmachine commented Sep 8, 2016

jasp00 commented Sep 9, 2016

zonkmachine commented Sep 9, 2016

zonkmachine commented Dec 22, 2016

zonkmachine commented Dec 23, 2016 •

edited

Loading

zonkmachine commented Jan 17, 2017

zonkmachine commented Jan 17, 2017

jasp00 commented Jan 20, 2017

zapashcanon Jan 21, 2017

zonkmachine Jan 21, 2017

zonkmachine commented Feb 3, 2017

zonkmachine commented Feb 3, 2017

zonkmachine Feb 3, 2017 •

edited

Loading

tresf Feb 3, 2017

zonkmachine commented Feb 3, 2017

zonkmachine commented Feb 4, 2017

tresf Feb 4, 2017

zonkmachine Feb 4, 2017

zonkmachine Feb 4, 2017

tresf Feb 4, 2017

zonkmachine Feb 4, 2017

zonkmachine commented Feb 8, 2017

tresf left a comment

tresf Feb 8, 2017

zonkmachine commented Feb 9, 2017

Beat note bug at the end of bars #2964

Beat note bug at the end of bars #2964

Conversation

zonkmachine commented Aug 8, 2016 • edited Loading

zonkmachine commented Aug 8, 2016

zonkmachine commented Aug 8, 2016

Umcaruje commented Aug 11, 2016

zonkmachine commented Aug 25, 2016

zonkmachine commented Aug 30, 2016

Umcaruje commented Aug 30, 2016

zonkmachine commented Aug 30, 2016

jasp00 commented Aug 30, 2016

zonkmachine commented Sep 8, 2016

jasp00 commented Sep 9, 2016

zonkmachine commented Sep 9, 2016

zonkmachine commented Dec 22, 2016

zonkmachine commented Dec 23, 2016 • edited Loading

zonkmachine commented Jan 17, 2017

zonkmachine commented Jan 17, 2017

jasp00 commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonkmachine commented Feb 3, 2017

zonkmachine commented Feb 3, 2017

zonkmachine Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonkmachine commented Feb 3, 2017

zonkmachine commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonkmachine commented Feb 8, 2017

tresf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonkmachine commented Feb 9, 2017

zonkmachine commented Aug 8, 2016 •

edited

Loading

zonkmachine commented Dec 23, 2016 •

edited

Loading

zonkmachine Feb 3, 2017 •

edited

Loading