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

Handle shrank automation patterns correctly #4012

Merged
merged 1 commit into from Nov 30, 2017

Conversation

Projects
None yet
2 participants
@PhysSong
Member

PhysSong commented Nov 26, 2017

Fix automation processing for shrank patterns. Retain the behavior before #3382 for those patterns.
Fixes #3800.

@PhysSong PhysSong requested a review from lukas-w Nov 26, 2017

@lukas-w

Would it make sense to move the check inside AutomationPattern::valueAt(...) so the fix is available to all callers?

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Nov 26, 2017

Member

@lukas-w I've tried something similar. It needs additional checks to keep current behavior in irrelevant places(ex. automation editor, note detuning). So I just chose this way for simplicity.

Member

PhysSong commented Nov 26, 2017

@lukas-w I've tried something similar. It needs additional checks to keep current behavior in irrelevant places(ex. automation editor, note detuning). So I just chose this way for simplicity.

@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Nov 27, 2017

Member

@PhysSong What's the issue with e.g. note detuning? Is the pattern's length not set correctly?

Member

lukas-w commented Nov 27, 2017

@PhysSong What's the issue with e.g. note detuning? Is the pattern's length not set correctly?

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Nov 28, 2017

Member

What's the issue with e.g. note detuning? Is the pattern's length not set correctly?

Yes. I detuned a two-bar note in piano-roll and the automation pattern have points over one measure. However, Its length is 192 ticks long(e.g. one bar long). Here's the project file:
detuning.mmp.zip

Member

PhysSong commented Nov 28, 2017

What's the issue with e.g. note detuning? Is the pattern's length not set correctly?

Yes. I detuned a two-bar note in piano-roll and the automation pattern have points over one measure. However, Its length is 192 ticks long(e.g. one bar long). Here's the project file:
detuning.mmp.zip

@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Nov 30, 2017

Member

I see, thanks for explaining. I'd say we merge this and do further changes (if any) on master.

Member

lukas-w commented Nov 30, 2017

I see, thanks for explaining. I'd say we merge this and do further changes (if any) on master.

@lukas-w lukas-w merged commit ee9b593 into LMMS:stable-1.2 Nov 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

lukas-w added a commit that referenced this pull request Nov 30, 2017

Add more automation tests
See issue #3800 (Automations continue after the end of their TCOs) which
was fixed via #4012
@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Nov 30, 2017

Member

Merged and added unit tests for the bug via d146308.

Member

lukas-w commented Nov 30, 2017

Merged and added unit tests for the bug via d146308.

@PhysSong PhysSong deleted the PhysSong:autoshrink branch Nov 30, 2017

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Dec 1, 2017

Member

@lukas-w In d146308, passing double caused linkage issue with Qt4 build. Fixed via 6cc118c.

Member

PhysSong commented Dec 1, 2017

@lukas-w In d146308, passing double caused linkage issue with Qt4 build. Fixed via 6cc118c.

@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Dec 1, 2017

Member

Thanks @PhysSong. It's strange that the error only appears on macOS…

Member

lukas-w commented Dec 1, 2017

Thanks @PhysSong. It's strange that the error only appears on macOS…

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Dec 1, 2017

Member

Unit test is disabled in Windows builds, and Linux Qt4 build is missing now. So only Mac + Qt4 fails.

Member

PhysSong commented Dec 1, 2017

Unit test is disabled in Windows builds, and Linux Qt4 build is missing now. So only Mac + Qt4 fails.

@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Dec 1, 2017

Member

Of course!

Member

lukas-w commented Dec 1, 2017

Of course!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment