Rosegarden / Bugs / #1464 tuplet of 7 in one beat leads to incorrect durations (spurious rests), cannot insert all 7

D. Michael McIntyre - 2015-11-10

Tom Breton was working on the big-tuplet-rewrite branch, which totally gutted and rebuilt the way Rosegarden handles tuplets. He was going to merge it whenever he was satisfied with it, and that's the last I've heard of it in about two years.

It's probably worth having a look to see if his branch can handle this. If it can, then we should probably wake him up and see what the plan is here, if any. I assume the whole thing got paused at some point, and never finished baking. It happens.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

David Faure - 2015-11-11

Indeed, in that branch the insertion of the 7-for-4 works perfectly.

On the other hand, a 6-for-4 tuplet (of 16ths) leads to 3+3, while I want to see a single number '6', and this works in trunk. So that's a regression in the branch (at least it shows something that is technically correct, but it's a bit too automatic to my taste, if I say 6 I want to see 6).

I have no idea if it's easier to fix the 7-for-4 issue in trunk or to fix the 6-for-4 issue in big-tuplet-rewrite. I also don't know what else big-tuplet-rewrite might break :). Feel free to merge that branch or ping Tom, I can look into fixing the remaining bugs once you guys have decided which tuplet logic to use.

The GUI around editing tuplets has the same bugs as in trunk (which is good, it means I can look into fixing that independently).

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Philip Leishman - 2021-06-26

Had a look at this - all very complicated,
I made a small change to NoteRestInserter which seems to make things look better.
According to the comment this is something to do with inserting triplets at the end of a segment and indeed that seems to fail now.
Does anyone know anything about this ?
I have made a merge request for the change.

https://sourceforge.net/p/rosegarden/git/merge-requests/23/

Last edit: Ted Felix 2021-06-27

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-06-27

Yeah, I've never ventured down this path. It appears as if there are at least three ways to get a septuplet:

Use the Tuplet Insert Mode (;) which is the "-?-" button below the triplet button. That seems a bit finicky as reported here.

Phrase > Tuplet (Ctrl+T) can fix 7 arbitrary notes e.g. 16ths and make them a 7 in 4 septuplet.

Adjust > Quantize (=) and select grid or legato quantizer which allow for specifying an arbitrary "base grid unit". In this case, specify 37 for 7 in the time of 4. This assumes you are starting with a performance of 7 in the time of 4 and want to make it perfect.

All these alternatives might be why this was dropped. One way or another this can be done.

At any rate, I was able to reproduce the original issue. I will have a look at the change. I wonder if there is any value in trying to bring the tuplet rewrite branch back to life from svn. I doubt it will cleanly apply at this point, but you never know.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-06-27

Change looks good. I'm going to whip up a test plan and maybe we can use that to drive improvements? It would be nice to know what's working and what isn't.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-06-27

Ok, I've thrown together the beginnings of a tuplet test plan:

https://www.rosegardenmusic.com/wiki/test:notation_tuplet

It looks like your change fixes 7 in 4 and 6 in 4. 5 in 4 also works fine. So this appears to be a big improvement.

I wasn't able to figure out the failing case that you mentioned:

According to the comment this is something to do with inserting triplets at the end of a segment and indeed that seems to fail now.

What is this case?

And what other cases should we add to the test plan?

Last edit: Ted Felix 2021-12-28

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Philip Leishman - 2021-06-27

Good start with the test plans - automated testing would be great. At work we used squish from froglogic for automated testing. Very good but has to be bought.
I looked into the tuplet rewrite branch but it seems to be on a VERY old code base - I don't think it helps much. It seems to me that the tuplet code was broken by a bug fix for triplets at the end of a segment.
To reproduce:
create a segment (just 1 bar)
enter tripet mode -3- and select quavers
press keys asd repeatedly.
All the triplets are fine until the last one in the segment.
Maybe look for a better fix for this .....

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-06-27

Ah, I see. This is probably related to the cursor movement fix from before. I added the test case to the wiki so we can track it.

So what's the plan? Did you want to keep working on this? Would a round of cleanup help? I'm always up for doing some cleanup.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Philip Leishman - 2021-06-27

I will continue looking into this.
Not sure what should be cleaned up at the moment !!

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Philip Leishman - 2021-06-27

assigned_to: Philip Leishman
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Philip Leishman - 2021-07-04

So I have pushed a fix for the triplet at end of segment.
The tuplet code is rather complex and confusing.
Maybe some comments need cleaning up.....

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Philip Leishman - 2021-07-05

While testing I noticed the tuplet beam for 5/4 tuples was disappearing when the tuplet was complete.
I have pushed a fix for this.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-07-11

Looks good. Thanks. I pushed to master.

I added the Tuplet stuff to my cleanup todo list. Are there any specific classes or functions that need attention? I noticed that you touched these areas:

SegmentNotationHelper::makeTupletGroup()

NotationGroup

NoteRestInserter

Are there others?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-07-11

status: open --> feedback
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Philip Leishman - 2021-07-11

Those are the places that need looking at. Maybe TupletCommand too.
I can't pretend to understand all the tuple code - maybe worth looking at the big-tuplet-rewrite code. But as I said it seems to be on a very old code base. Lots of classes which don't exist anymore!

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-11-26

@lorenzosu has found a regression that might be related to this. I've not bisected to make sure. I've added the test case to the test plan:

https://www.rosegardenmusic.com/wiki/test:rosegarden_test_plans#triplets_before_quarter

In 21.06, this works as expected. Now the quarter note on bar 2 is somehow included in the triplet.

@lman, will you have time to look at this before December 8?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-11-26

[d7a523] introduces the problem. Looks like this was fixed once before in [#629].

Related

Bugs: ~~#629~~
Commit: [d7a523]

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-11-26

Ok, setting groupHasTimingAlready to true fixes the problems, but now 7 in 4 fails to work. I'm going with this for the time being. Unless Philip has some time to come up with something better before December 8.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-11-26

Pushed the "fix" as [8a266e]. 7 in 4 is now broken, but all the other test cases work fine. Please test latest git.

Related

Commit: [8a266e]

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-11-26

labels: --> notation

Priority: 1 --> 5
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Description has changed:

Diff:

--- old
+++ new
@@ -4,7 +4,7 @@

 Testcase http://www.davidfaure.fr/2015/septolet.rg

-The expected positions would be 3840, 3977, 4114, 4251, *4388*, 4525, 4662, 4800.
-Instead I end up with 3840, 3977, 4114, 4251, *4320*, etc.
+The expected positions would be 3840, 3977, 4114, 4251, ***4388***, 4525, 4662, 4800.
+Instead I end up with 3840, 3977, 4114, 4251, ***4320***, etc.

-Note sure where to debug this further, input welcome.
+Not sure where to debug this further, input welcome.

status: feedback --> open

Philip Leishman - 2021-11-28

Hmmm... setting groupHasTimingAlready was key to getting the 7 in 4 to work. I will look at it again but no guarantee for the next release.
Btw. I automated the test using xdotool. It works for me but may be dependent on settings, window size etc. I have attached the script if someone wants to try it. There are better automated test tools - squish is very good but has to be bought.

tuplet_test.rb

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Philip Leishman - 2021-12-22

OK - new release - new attempt to get this working.
New approach - It may be a case of sloppy note duration calculation and pointer positioning.
I have made a new version. It seems to get everything right except for a silly little rest at the end of the tuplet.
The calculation of note position and duration is a bit messy - better suggestions welcome!
See the new merge request for this bug.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ted Felix - 2021-12-23

Just ran the test plan. It appears to have problems with the same two test cases as 21.12: "7 in 4" and "Triplets Delete Triplets". I'm going to hold off on merging until we've got one or both of those two working. Let me know if there is another test case we are missing that is fixed by these latest changes.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Philip Leishman - 2021-12-23

Do you mean 20.12 or 21.06 ?
Strange. When I run the test the 7 in 4 looks quite good except for a superfluous mini-rest after the septuplet !!
I missed the "Triplets Delete Triplets" problem - it must be a new test case. Maybe a different issue - I will look into that too.
Can you recheck the 7 in 4 case again ?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

tuplet of 7 in one beat leads to incorrect durations (spurious rests),...

Group

Searches

Help

#1464 tuplet of 7 in one beat leads to incorrect durations (spurious rests), cannot insert all 7

Discussion

Related

Related