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

Fix stuck notes with Helm VSTi #5365

Merged
merged 1 commit into from
Jan 8, 2020
Merged

Conversation

DomClark
Copy link
Member

@DomClark DomClark commented Jan 5, 2020

Fixes #3640.

As mentioned in that issue, resizing notes from MIDI input during tempo changes causes overflows, which result in incorrect MIDI messages sent to the plugin. This is avoided by not resizing these notes.

@DomClark DomClark added needs style review A style review is currently required for this PR needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Jan 5, 2020
@JohannesLorenz
Copy link
Contributor

Initial tests looked/sounded good. However, when using arpeggio with long notes (e.g. 1/1), changing tempo did reset the arpeggio. Is this intended?

@DomClark
Copy link
Member Author

DomClark commented Jan 6, 2020

However, when using arpeggio with long notes (e.g. 1/1), changing tempo did reset the arpeggio. Is this intended?

Is this a regression in this PR? It's quite possibly unintended, but may be out of scope here. The arpeggiator is rather buggy anyway.

@JohannesLorenz
Copy link
Contributor

Is this a regression in this PR? It's quite possibly unintended, but may be out of scope here. The arpeggiator is rather buggy anyway.

Totally agree. And it's no regression.

I just wondered that the Arpeggiator case is not in the code, but somehow the original bug does not occur in Arpeggio (for whatever strange reason...). Was there a reason why you did not catch that case? Would it hurt to catch it?

@DomClark
Copy link
Member Author

DomClark commented Jan 7, 2020

The root cause of the issue is that notes from MIDI/keyboard input are implemented as normal notes but with a length of std::numeric_limits<f_cnt_t>::max() / 2. This causes m_origFrames * m_origTempo to overflow in NotePlayHandle::resize. Stacked notes inherit their length from their parent, so suffer the same problem, but arpeggio notes are always a sane length (a maximum of two seconds), so don't need special handling.

@JohannesLorenz
Copy link
Contributor

OK, makes sense.

All reviews passed, I'll merge this.

@JohannesLorenz JohannesLorenz merged commit c52682d into LMMS:stable-1.2 Jan 8, 2020
@DomClark DomClark deleted the fix-3640 branch March 12, 2021 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants