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

same note layering when sustain pedal is pressed #3774

Merged
merged 1 commit into from Aug 26, 2017

Conversation

@serdnab
Copy link
Contributor

@serdnab serdnab commented Aug 24, 2017

fix for #3757
I moved the signal emission for note ended to where all notes (sustained and not sustained) go, the release process on NotePlayHandle:play

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Aug 24, 2017

Haven't tested yet, but looks good. I'll test it soon.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Aug 24, 2017

Tested. It works great, and I can't find any regressions caused by this.

Copy link
Member

@zonkmachine zonkmachine left a comment

Tested summarily. Works like a charm!

@@ -252,8 +252,17 @@ void NotePlayHandle::play( sampleFrame * _working_buffer )
if( m_released && (!instrumentTrack()->isSustainPedalPressed() ||
m_releaseStarted) )
{
m_releaseStarted = true;
if (m_releaseStarted == false)

This comment has been minimized.

@zonkmachine

zonkmachine Aug 24, 2017
Member

Don't mix styles. This would be comparably to the rest of your changes in this PR and surrounding code.

if( m_releaseStarted == false )

This comment has been minimized.

@PhysSong

PhysSong Aug 24, 2017
Member

if (a) is new convention, if( a ) is old one. Many things are written with old convention, which make the code inconsistent for recently edited files.
I suggest using new style in line 252.

This comment has been minimized.

@serdnab
Copy link
Contributor Author

@serdnab serdnab commented Aug 25, 2017

So, I have to change something?

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Aug 25, 2017

It'd be better to either change line 252 and 258 to new style, or use old convention in line 255.

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

@zonkmachine zonkmachine commented Aug 26, 2017

So, I have to change something?

We had a discussion on this over at the discord developer channel. Since the conventions have changed the code base is inconsistent anyway. Merged. Thanks for fixing this!

zonkmachine added a commit that referenced this pull request Aug 26, 2017
[cherry-picked from master]
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Aug 26, 2017

Backported in efd0d34

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Aug 26, 2017

An oops with this PR. Recording single streamed instruments just disappeared...

@serdnab
Copy link
Contributor Author

@serdnab serdnab commented Aug 28, 2017

An oops with this PR. Recording single streamed instruments just disappeared...

@PhysSong I will use your proposed solution (here #3757 (comment)).
Do you want to code yourself?
If not, I will be glad of doing it myself.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Aug 28, 2017

I'm going to fix #3777. I'll try to fix this too.
If I say I've failed, please do this @serdnab.
Edit: #3777 is tricky to fix, so fix it if you can.

@serdnab
Copy link
Contributor Author

@serdnab serdnab commented Aug 29, 2017

I will take a look at #3777,

An oops with this PR. Recording single streamed instruments just disappeared...

and are you on this?

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Aug 29, 2017

I will take a look at #3777,

👍

and are you on this?

@PhysSong Is looking at this one. Maybe open a separate issue?

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Aug 30, 2017

Maybe open a separate issue?

👍 I think so.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Sep 2, 2017

I tried my suggestion in #3757 (comment). It seems to fix both issues, but it is an incomplete solution for #3777.

@serdnab
Copy link
Contributor Author

@serdnab serdnab commented Sep 4, 2017

@PhysSong I have a solution for #3777 but I'm waiting your PR because I will use the place where you store the sustained notes.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Sep 4, 2017

@serdnab What is your solution for #3777?

@serdnab
Copy link
Contributor Author

@serdnab serdnab commented Sep 6, 2017

@PhysSong

What is your solution for #3777?

Assign an unique ID for every recording instance of Note and then on finishRecordNote() at PianoRoll.cpp compare by that ID.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Sep 7, 2017

@serdnab That can be a permanent and fundamental solution, if the uniqueness is guaranteed and the ID won't be changed. I'll send a PR for single-stream recording fix.

@serdnab
Copy link
Contributor Author

@serdnab serdnab commented Sep 9, 2017

The problem is not that instruments are single-stream, it is that they don't have a release.
Some instruments, even if they have envelope off, have an internal release.
Try with Vibed instrument and you will see that when envelope is on it records normally.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Sep 9, 2017

Try with Vibed instrument and you will see that when envelope is on it records normally.

Confirmed. sfxr too.

@zonkmachine zonkmachine mentioned this pull request Sep 9, 2017
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Sep 9, 2017

An oops with this PR. Recording single streamed instruments just disappeared...

@PhysSong Is looking at this one. Maybe open a separate issue?

The problem is not that instruments are single-stream, it is that they don't have a release.
Some instruments, even if they have envelope off, have an internal release.

I don't have time to write up an issue for this right now. RC4?

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Sep 9, 2017

I will open a PR that fixes recording single-stream instruments tomorrow. RC4 is really coming :)

PhysSong added a commit to PhysSong/lmms that referenced this pull request Sep 11, 2017
PhysSong added a commit to PhysSong/lmms that referenced this pull request Sep 11, 2017
PhysSong added a commit to PhysSong/lmms that referenced this pull request Sep 18, 2017
Umcaruje added a commit that referenced this pull request Sep 18, 2017
)

* Revert "same note layering when sustain pedal is pressed (#3774)"

This reverts commit e387e77.

* Fix recording of sustained notes
gi0e5b06 added a commit to gi0e5b06/lmms that referenced this pull request Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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