Skip to content
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

Midi sustain working when envelope is on #3730

Merged
merged 3 commits into from Aug 2, 2017
Merged

Conversation

@serdnab
Copy link
Contributor

@serdnab serdnab commented Jul 26, 2017

Fixes #3537.
When sustain pedal is pressed NotePlayHandle::play() doesn't enter release phase.
And in InstrumentSoundShaping::processAudioBuffer(), the envelope's release is posponed until the sustain pedal is released.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 26, 2017

Cool!

Issue: When I take a note with long decay, I release by letting go of the sustain pedal and, while it's decaying, hold it at a new level by pressing the sustain again. I don't think this is wanted behaviour for a synth although it very well could be how some machines work.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 28, 2017

I think it would have been easier if the sustain pedal stuff had been all done in InstrumentTrack.cpp from start.

@serdnab
Copy link
Contributor Author

@serdnab serdnab commented Jul 29, 2017

When I take a note with long decay

Do you mean long release?

I don't think this is wanted behaviour for a synth

Do you want that when the sustain pedal is pressed again, it doesn't affect the release (that keeps fading out to silence)?

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 29, 2017

Do you mean long release?

Oops, yes... :)

Do you want that when the sustain pedal is pressed again, it doesn't affect the release

Yes. If you let go of the sustain pedal, all keys that aren't pressed down should go into release and if you press down the sustain again this shouldn't hold released notes again. This is my understanding of how a common envelope generator should behave.
What does @LMMS/developers say?

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jul 29, 2017

Well, I don't know the common behavior. However, I think that sustain pedal shouldn't pause releasing.
Actually I have another solution for this issue, though it isn't tested.

@serdnab
Copy link
Contributor Author

@serdnab serdnab commented Jul 30, 2017

Done, now release isn't paused when sustain pedal is pressed again.

Copy link
Member

@zonkmachine zonkmachine left a comment

This works fine. I approve this PR with my suggested changes.

@@ -137,7 +137,8 @@ void InstrumentSoundShaping::processAudioBuffer( sampleFrame* buffer,
const f_cnt_t envTotalFrames = n->totalFramesPlayed();
f_cnt_t envReleaseBegin = envTotalFrames - n->releaseFramesDone() + n->framesBeforeRelease();

if( n->isReleased() == false )
if( n->isReleased() == false || ( n->instrumentTrack()->isSustainPedalPressed() &&

This comment has been minimized.

@zonkmachine

zonkmachine Jul 31, 2017
Member

I think you should adapt the original code to your format.
n->isReleased() == false --> !n->isReleased()

bool isReleaseStarted() const
{
return m_releaseStarted;
}

This comment has been minimized.

@zonkmachine

zonkmachine Jul 31, 2017
Member

you stick this to the end of NotePlayHandle but it belongs, logically, together with isReleased().
Please move isReleaseStarted() to after isReleased() and m_releaseStarted to after m_released.

@zonkmachine zonkmachine added this to the 1.2.0 milestone Jul 31, 2017
@zonkmachine zonkmachine added this to In Progress in Release 1.2.0 RC4 Jul 31, 2017
@serdnab
Copy link
Contributor Author

@serdnab serdnab commented Aug 2, 2017

Done, addressed your indications.

@zonkmachine zonkmachine merged commit 44028c8 into LMMS:master Aug 2, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PhysSong
Copy link
Member

@PhysSong PhysSong commented Aug 2, 2017

@zonkmachine Are you going to backport this?

Edit: Done via 31126b0, by @zonkmachine.

zonkmachine added a commit that referenced this pull request Aug 2, 2017
* midi sustain working when envelope is on

* pressing sustain pedal again doesn't pause release

[cherry-picked from master]
@zonkmachine zonkmachine moved this from In Progress to Done in Release 1.2.0 RC4 Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.