Arpeggiator sometimes trigger one note too many

[zonkmachine]

As reported on our Discord support channel.
Test project by Discord member Half-Gray/7E7E7E
bass_test.mmp.zip

The arpeggiator sometimes triggers one arpnote to many at the end of a note with some settings.
Try turning up sustain a bit to trigger it.

The issue is there already in lmms-1.0.3 but not as obvious. In the test project you'll see a spike in the audio (mixer LEDs) when switching to a new held note. This goes away if you turn the Sustain and Release down to zero. The latest change happened in 67e9371 where the glitch becomes more prominent.

Edit: Superfluous bisect report moved to gist:
https://gist.github.com/zonkmachine/40e7000c2f93c68f0c55de2693ce27b0

[PhysSong]

This goes away if you turn the Sustain and Release down to zero.

lmms/src/core/InstrumentFunctions.cpp

Lines 344 to 350 in 5d90aec

	if( _n->origin() == NotePlayHandle::OriginArpeggio ||
	_n->origin() == NotePlayHandle::OriginNoteStacking ||
	!m_arpEnabledModel.value() ||
	( _n->isReleased() && _n->releaseFramesDone() >= _n->actualReleaseFramesToDo() ) )
	{
	return;
	}

It looks like the logic uses (note length + release frames) instead of the real length. I think there's no reason to check release frames here.

[PhysSong]

Seems like LMMS plays arpeggio for released note and multiply release envelope value to new notes' volume. I don't know why/if we should...

lmms/src/core/InstrumentFunctions.cpp

Lines 505 to 508 in 5d90aec

	if( _n->isReleased() )
	{
	vol_level = _n->volumeLevel( cur_frame + gated_frames );
	}

[zonkmachine]

Personally I haven't seen this problem a lot and thought that it had been solved simply because I've grown into the habit of using the same staccato like envelope with all of my arpeggios that wouldn't trigger the issue.

It looks like the logic uses (note length + release frames) instead of the real length. I think there's no reason to check release frames here.

Yes. Just changing

( _n->isReleased() && _n->releaseFramesDone() >= _n->actualReleaseFramesToDo() ) ) 

to

_n->isReleased() )

seem to fix this.

[zonkmachine]

lmms/src/core/InstrumentFunctions.cpp

Lines 505 to 508 in 5d90aec

	if( _n->isReleased() )
	{
	vol_level = _n->volumeLevel( cur_frame + gated_frames );
	}

Right, you've got good eyes. This decay is for the releaseFrames (post above) giving a sort of pseudo delay end to an arpeggio with a long decay. I think it should just be removed along with the other change above (which makes it never execute anyway).