Fix for piano roll bug (1.0.93 - Play is silent inside piano-roll #1110)

[krafczyk]

Do not set the bbtrack for a note handle by searching for a bbtrack by tco_num. The bbtrack index and tco_num are unrelated.

[tresf]

Addresses #1110.

What a terrific addition to stable-1.1, also terrific explanation and documentation.

I'll let Vesa or Toby approve this one.

[diizy]

This seems like it could cause other issues. With this commit, it looks like the bbTrack property isn't being set for the nph at all.

[diizy]

I don't dare merge this as it seems like it would break the playback of bb tracks...

[tresf]

I don't dare merge this as it seems like it would break the playback of bb tracks...

Hmm... If you're wrong this would be a huge win as this bug dates back several years. Shall I make a build and have our testers hash it out?

[diizy]

I think the bbtrack has to be set for the nph, otherwise there's other things that may break. I haven't really delved too deeply with the bbtrack functionality yet (saving that can of worms for later), but it really seems like there could be problems here.

Actually, also, the tco_num and bbtrack index are not unrelated, as far as I can tell. The way bbtracks work is a bit complicated... but basically, each track that are in the bb-editor, has tcos for each existing bb-track, and the tco number of the patterns on each track correspond to the bb tracks, so I believe the bb track property is being set correctly here.

If the problem is in piano roll behaviour, then I suggest fixing the problem in piano roll, not here...

[diizy]

I think we rather have to change the playback function of the instrument track... Now this is just a hunch, but I think the problem here might be that we're calling InstrumentTrack::play the same way when playing a single pattern and when it's being called from a bb_track.

Basically, we need a way to differentiate patterns that belong to a bb track from patterns that don't.

[krafczyk]

@diizy, Thanks for being skeptical of my commit. I've explored the code again, and now I'm positive that it's the right thing to do. In fact, I've now removed completely the dependence of play handles directly on bb tracks. I'll explain below, and address your comments.

I don't dare merge this as it seems like it would break the playback of bb tracks...

This is the first thing I checked. It does not affect the playback of bb tracks.

Actually, also, the tco_num and bbtrack index are not unrelated, as far as I can tell.

How do you mean? I understand from making print statements that the tco number is just the index of 'blocks' on a given track, and the total number may be different for each track. Just given this, I don't see how they could be related, however I can prove it more rigorously.

Upon furthur analysis of the code, Muting and soloing is governed by the 'track' classes. I'll take Muting as an example. When a track is played, the overload for that track will be played. i.e. for an instrument track, the InstrumentTrack::play function will be executed. For a bb track, the bbtrack::play function will be executed.

1. If you look at bb_track.cpp:410 (on my branch) You can see that the 'isMuted' property for the bbtrack is tested at that moment.
2. Then bbTrackContainer::play is called at bb_track.cpp:417.
3. Inside bbTrackContainer::play, at bb_track_container.cpp:71, the tracks contained inside the bbtrack are iterated over and their respective ::play functions are called.
4. If we now inspect InstrumentTrack::play at InstrumentTrack.cpp:160 you can see that the individual track is tested to see if it has been muted.

I encourage skeptics to checkout, build and test my branch. Prove to yourself that it works.

If the problem is in piano roll behaviour, then I suggest fixing the problem in piano roll, not here...

I don't think this is the case. As stated above, the muting/soloing is taken care of by the tracks themselves. The play from piano roll does not have any bearing on this.

Along these lines, I've opted to remove entirely the mention of bbtracks from the various play handles. This will prevent confusion in the future.

I'm 99% confident about this, However; it's possible I'm mistaken; but again test the code yourself. Nothing seems to be out of place to me, both soloing and muting work as expected.

Also, is the person who wrote this code still around?

[diizy]

On 09/13/2014 01:10 AM, Matthew Scott Krafczyk wrote:

@diizy https://github.com/diizy, Thanks for being skeptical of my
commit. I've explored the code again, and now I'm positive that it's
the right thing to do. In fact, I've now removed completely the
dependence of play handles directly on bb tracks. I'll explain below,
and address your comments.

I don't dare merge this as it seems like it would break the
playback of bb tracks...

This is the first thing I checked. It does not affect the playback of
bb tracks.

Actually, also, the tco_num and bbtrack index are not unrelated,
as far as I can tell.

How do you mean? I understand from making print statements that the
tco number is just the index of 'blocks' on a given track, and the
total number may be different for each track. Just given this, I don't
see how they /could/ be related, however I can prove it more rigorously.

I explained it in my earlier post, but maybe I wasn't clear enough. The
way bb tracks work is that each track in the bb-editor has a number of
TCOs (ie. patterns or "blocks" as you call them) but, since the
bb-editor must have a different pattern for each bb-track, they're
divided accross the track so that TCO #0 is the pattern for bb track #0,
TCO #1 is the pattern for bb track #1 etc.

Since every bb track only has one pattern for each of the tracks in the
bb-editor, the patterns used by each track can be matched to the tracks
by the TCO number. Does this make things clearer?

Upon furthur analysis of the code, Muting and soloing is governed by
the 'track' classes. I'll take Muting as an example. When a track is
played, the overload for that track will be played. i.e. for an
instrument track, the InstrumentTrack::play function will be executed.
For a bb track, the bbtrack::play function will be executed.

1. If you look at bb_track.cpp:410 (on my branch) You can see that
   the 'isMuted' property for the bbtrack is tested at that moment.
2. Then bbTrackContainer::play is called at bb_track.cpp:417.
3. Inside bbTrackContainer::play, at bb_track_container.cpp:71, the
   tracks contained inside the bbtrack are iterated over and their
   respective ::play functions are called.

Right, and at this point, the tco_number that matches the bb_track
number is passed to InstrumentTrack::play, in which it is used to
determine which TCO it should play from the InstrumentTrack.

Along these lines, I've opted to remove entirely the mention of
bbtracks from the various play handles. This will prevent confusion in
the future.

I'm not really comfortable with this. I believe there are some functions
which depend on this for certain things, eg. getting all active
PlayHandles associated with a given bb track. Also, there may be future
features which may need this functionality, and removing it now might
place limitations on those.

I'd recommend a different way for solving this issue, for instance:
modifying the code that calls InstrumentTrack::play from song.cpp (or
wherever) when a single pattern is played so that it's distinct from the
bb-track call and thus making it so that the NPH's won't be erroneously
assigned bb tracks.

[krafczyk]

Since every bb track only has one pattern for each of the tracks in the
bb-editor, the patterns used by each track can be matched to the tracks
by the TCO number. Does this make things clearer?

Yes, much clearer.

Right, and at this point, the tco_number that matches the bb_track
number is passed to InstrumentTrack::play, in which it is used to
determine which TCO it should play from the InstrumentTrack.

Okay, so then the tracks don't actually need to save bbtrack information. This information is passed directly to the track.

I believe there are some functions
which depend on this for certain things, eg. getting all active
PlayHandles associated with a given bb track

Isn't this handled by bbTrackContainers? Part of the point of removing the references to bbtracks from play handles is to show that nobody actually uses these references, except in this single instance that causes the bug.

Also, there may be future features which may need this functionality, and removing it now might
place limitations on those.

I'm not dead set on removing completely the bbtrack stuff, so I'll remove it if you really want me to, but I think the code will be simpler this way.

I'd recommend a different way for solving this issue, for instance:
modifying the code that calls InstrumentTrack::play from song.cpp (or
wherever) when a single pattern is played so that it's distinct from the
bb-track call and thus making it so that the NPH's won't be erroneously
assigned bb tracks.

This is almost certainly harder and will increase code complexity just to avoid removing the bbtrack stuff, which as I pointed out isn't actually used by anybody (except to cause this bug).

The minimal way to go is to take my first commit and remove the bbtrack lines which directly cause the bug.

[diizy]

On 09/13/2014 02:14 AM, Matthew Scott Krafczyk wrote:

Since every bb track only has one pattern for each of the tracks
in the
bb-editor, the patterns used by each track can be matched to the
tracks
by the TCO number. Does this make things clearer?

Yes, much clearer.

Right, and at this point, the tco_number that matches the bb_track
number is passed to InstrumentTrack::play, in which it is used to
determine which TCO it should play from the InstrumentTrack.

Okay, so then the tracks don't actually need to save bbtrack
information. This information is passed directly to the track.

The information is passed to the track, but in a sort of flawed way
because when you play only a single pattern (eg. in piano roll), the tco
number is also passed, which causes the confusion because the track
cannot know if it's being given the bb track number or just a pattern
number because it's being called from piano roll.

I believe there are some functions
which depend on this for certain things, eg. getting all active
PlayHandles associated with a given bb track

Isn't this handled by bbTrackContainers?

No, it's not, because the function may need to be generic and work for
any track, including bb tracks. NotePlayHandle has a function
isFromTrack which is used to determine which track the NPH belongs to.
Which is in turn used by another function in Mixer which gathers all
NPH's that belong to a certain track (instrument or bb). This is then
used eg. when we destroy an instrument and need to get all playhandles
of the track to destroy them too.

Part of the point of removing the references to bbtracks from play
handles is to show that nobody actually uses these references, except
in this single instance that causes the bug.

They're used in NotePlayHandle::isFromTrack() which checks both the
NPH's associated instrumentTrack and bbTrack (if any). This is why I'm
concerned that there could be adverse effects. I'm not exactly sure if
there are cases where we need to gather all playhandles that belong to a
bbTrack, but I'm also not entirely comfortable with removing the
possibility to do so. Seems like this is the kind of decision that could
come back and bite us in the ass later.

[krafczyk]

No, it's not, because the function may need to be generic and work for
any track, including bb tracks.
...
They're used in NotePlayHandle::isFromTrack() which checks both the
NPH's associated instrumentTrack and bbTrack (if any). This is why I'm
concerned that there could be adverse effects. I'm not exactly sure if
there are cases where we need to gather all playhandles that belong to a
bbTrack, but I'm also not entirely comfortable with removing the
possibility to do so.

Given these considerations, I will attempt to find another way.

Here's a question though, Is it possible to have negative tco number? A possible solution is to use negative tco numbers for bbtrack tcos, but there is code in InstrumentTrack::play which uses -1 for something. if we start from -2, it might be possible to do that, otherwise I need to think.

[diizy]

Sorry, I thought I answered this already. -1 is used to mean that we want to play all TCOs within a certain range, not a specific TCO - this is used for normal tracks when playing the song in song editor.

I think rather than use "magic numbers" it'd be better to just add a boolean parameter to the play() function, note that this should be added to track because play() is inherited from track, and to all track types as well.

[diizy]

Since this part of the software is getting redesigned in the LMMS 2.0 effort, this issue will become moot. Thus I'm closing this pull request, but I hope you'll join us in planning the future of LMMS - there will be plenty of work to do soon in the 2.0 branch...

[tresf]

Since this part of the software is getting redesigned in the LMMS 2.0 effort, this issue will become moot. Thus I'm closing this pull request, but I hope you'll join us in planning the future of LMMS - there will be plenty of work to do soon in the 2.0 branch...

Ok, but from a life-cycle perspective, we have to be careful not to close out too many stable bug reports long before the code commits have happened. I would recommend for the majority of items, we instead label the 1.2 affecting bugs as 1.2 and close them in bulk when the time is right.

The reason for this is that if the project suffers a misfortune and the 2.0 branch becomes stale, then the project can suffer quality control and tracking issues for the branch that remains. 👍

[diizy]

On 11/26/2014 08:11 PM, Tres Finocchiaro wrote:

Ok, but from a life-cycle perspective, we have to be careful not to
close out too many stable bug reports

This was a pull request, not a bug report...

[tresf]

This was a pull request, not a bug report...

Doh! 😊

[Umcaruje]

And now @diizy went away, the 2.0 effort is dead, and we still have this bug. In the future I think we should stay away from these kinds of "solutions", and actually fix the problem when there is a willing developer to work on the issue, rather than sweeping this under a rug.