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

Include past automation patterns in processing #3382

Merged
merged 11 commits into from Mar 26, 2017
Merged

Include past automation patterns in processing #3382

merged 11 commits into from Mar 26, 2017

Conversation

@lukas-w
Copy link
Member

@lukas-w lukas-w commented Feb 23, 2017

Closes #662.

lmms-662

I think that ideally, automated values should update when setting the playing position, not just when playing. Opinions on that?

@lukas-w lukas-w changed the title Include past automation tracks in processing Include past automation patterns in processing Feb 23, 2017
@tresf
Copy link
Member

@tresf tresf commented Feb 23, 2017

Holy mackerel! 🎉

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Mar 7, 2017

Tested. 👍

I think that ideally, automated values should update when setting the playing position, not just when playing. Opinions on that?

Probably yes to this but there are other cases where values aren't updated until you press play. For instance when you import a midi file and it's tempo isn't updated until you play.

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Mar 7, 2017

Unfortunately this PR causes an issue where some automation patterns are ignored, because automation tracks are processed in the order they appear in the song editor. See the following screenshot, where the second track overwrites the first one:

image

For instance when you import a midi file and it's tempo isn't updated until you play.

Isn't that just a global automation track on the tempo? Doesn't sound like desired behavior to me.

@lukas-w lukas-w force-pushed the iss-662 branch from f559b3e to c42eb2e Mar 7, 2017
@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Mar 7, 2017

some automation patterns are ignored

Fixed via c42eb2e.

I did a quick test implementing "Updating when setting the position", but I can't seem to find a good place in the code to insert the updating procedure at.
lmms-662-2

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Mar 9, 2017

Do we want to include this PR in 1.2?

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Mar 9, 2017

Do we want to include this PR in 1.2?

I really like this feature, but we should be in a feature freeze, so not really sure. Interested in what others have in mind. I'm fine either way really.

@tresf
Copy link
Member

@tresf tresf commented Mar 9, 2017

Do we want to include this PR in 1.2?

It's a pretty easy fix to revert if it causes problems. If someone can provide some (additional) testing, that would certainly help. @musikBear is usually good for this if we can get him a win32 installer.

@musikBear
Copy link

@musikBear musikBear commented Mar 10, 2017

@lukas-w Marvellous 🎆

@musikBear
Copy link

@musikBear musikBear commented Mar 10, 2017

@lukas-w Marvellous 🎆
That one is indeed a good feature.
If its decided that it goes past the freeze, and into 1.2, just '@' me a notification for the test-file

Copy link
Member

@jasp00 jasp00 left a comment

Some issues to address. Then, this should be merged.

@@ -97,6 +97,7 @@ class EXPORT Song : public TrackContainer


void processNextBuffer();
void processAutomations(const TrackList& tracks, MidiTime timeStart, fpp_t frames, int tcoNum);

This comment has been minimized.

@jasp00

jasp00 Mar 17, 2017
Member

Method should be private.

This comment has been minimized.

@lukas-w

lukas-w Mar 18, 2017
Author Member

I'm going to add a unit test for this feature, so I'll leave it public. We can gladly refactor to split this into a public and a private part some day.

This comment has been minimized.

@jasp00

jasp00 Mar 19, 2017
Member

If this is really private, you should not be testing private methods. You should publish when necessary.

This comment has been minimized.

@lukas-w

lukas-w Mar 19, 2017
Author Member

you should not be testing private methods

Good thing this one's public then. 😉 As I said, we can refactor this some day, but it's out of scope for this PR.

This comment has been minimized.

@jasp00

jasp00 Mar 20, 2017
Member

Why cannot processAutomations() be private until there is a real need to be public? May I do the cut+paste?

@@ -146,6 +146,8 @@ class TrackContentObject : public Model, public JournallingObject
return m_selectViewOnCreate;
}

/// Returns true if and only if a->startPosition() < b->startPosition()
static bool comparePosition(const TrackContentObject* a, const TrackContentObject* b);

This comment has been minimized.

@jasp00

jasp00 Mar 17, 2017
Member

This is only needed inside Song and uses a public TrackContentObject function. It should a static function in Song.cpp.

This comment has been minimized.

@lukas-w

lukas-w Mar 18, 2017
Author Member

We may end up using it elsewhere. Also, it's quite obviously a function related to TrackContentObject, so I don't think moving it to a mostly unrelated file is a good idea. Whether TrackContentObject should know it's own position is debatable of course, but that's how it is right now.

This comment has been minimized.

@jasp00

jasp00 Mar 19, 2017
Member

I do not agree, but your choice is not bad.

TrackContentObject* tco = track->getTCO(i);

// Skip this TCO if it's muted or not contained in the time period we're processing
if( tco == NULL || tco->isMuted() || tco->startPosition() > time_end)

This comment has been minimized.

@jasp00

jasp00 Mar 17, 2017
Member

tco == NULL is an error. It should not be hidden.

This comment has been minimized.

@lukas-w

lukas-w Mar 18, 2017
Author Member

I agree, but I didn't write that part, I just moved it and didn't dare remove the check introduced by @tobydox 9 years ago. Feel free to add something like a qCritical message or to just remove the check.

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Mar 21, 2017

Added tests and some refactorizations, which also makes processAutomations private, @jasp00.

@lukas-w lukas-w force-pushed the iss-662 branch from 4bce488 to b23041b Mar 21, 2017
@jasp00
Copy link
Member

@jasp00 jasp00 commented Mar 21, 2017

Good. Could you add the scope trick for automatedValuesAt()? Something like privateAndTest that is defined to private normally and to public on tests.

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Mar 22, 2017

Could you add the scope trick for automatedValuesAt()?

What? Why?

lukas-w added 2 commits Mar 22, 2017
# Conflicts:
#	include/AutomationPattern.h
#	src/core/AutomationPattern.cpp
#	src/core/Song.cpp
@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Mar 22, 2017

I think it's better to not merge this with 1.2. It may be a low risk, but we should take the term "feature freeze" seriously if we want to have a working 1.2 release any time soon.
I'll create a new stable-1.2 branch, then merge this into master.

@jasp00
Copy link
Member

@jasp00 jasp00 commented Mar 23, 2017

Perhaps you should not have added so many changes to the PR, but this is a bug fix. I do not know why #662 is tagged as "enhancement". After all your work, if you are not going to finish, I will.

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Mar 23, 2017

It may be a bug fix from the user perspective, but from the code perspective, it can be considered a rewrite of automation processing. So I suggest not merging this without extensive testing, which is better done in part of the next release so we don't hold up this one even longer.

if you are not going to finish, I will.

What's not finished here?

@tresf
Copy link
Member

@tresf tresf commented Mar 23, 2017

I do not know why #662 is tagged as "enhancement". After all your work, if you are not going to finish, I will.

I think @jasp00's point here (hard to read between the lines) is that if this is a bug, it can be merged as a bug-fix after the freeze... whereas as an enhancement, it would wait until the next release.

In regards to "why" this is an enhancement (I labeled it as such), it is because the automation behavior was behaving correctly, just not intuitively. e.g. the old behavior was not a bug and it was not wrong.

From a delivery perspective, this feature adds a tremendous amount of value to 1.2 so if we could get a precompiled version out for our testers, that could certainly influence the candidacy.

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Mar 23, 2017

@tresf
Copy link
Member

@tresf tresf commented Mar 23, 2017

Testing feedback from @specular:

@tresf 26 automation tracks and no playback issues. Tested reading backwards on a few and it works. 👌

On a side note, there are some items that are being duplicated, namely all instruments as well as the menu item for LADSPA Plugin browser. I can't see it being related, but wanted to relay that feedback.

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Mar 26, 2017

On a side note, there are some items that are being duplicated

This probably surfaced only now because plugin search paths were broken until 7251c84. Fixed via 0d77cef.

So it sounds like no one's objecting to having this in 1.2 (except me). Merging 👍

@lukas-w lukas-w changed the base branch from master to stable-1.2 Mar 26, 2017
@lukas-w lukas-w merged commit 6004eda into stable-1.2 Mar 26, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@lukas-w lukas-w deleted the iss-662 branch Mar 26, 2017
@jasp00
Copy link
Member

@jasp00 jasp00 commented Mar 28, 2017

Could you add the scope trick for automatedValuesAt()?
What?

privateAndTest:
    static AutomatedValueMap automatedValuesAt(const Track::tcoVector& tcos, MidiTime time);

Why?

Because automatedValuesAt() should not be used by other classes.

Tests should be under tests/src.

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Mar 28, 2017

Because automatedValuesAt() should not be used by other classes.

I understand what the private keyword is for. But there's no point in making this function private and introducing a "scope trick" doesn't exactly encourage good design either. It's a static, read-only function. No damage done with this one public.

Tests should be under tests/src.

They are.

@jasp00
Copy link
Member

@jasp00 jasp00 commented Mar 28, 2017

No damage done with this one public.

How does the "scope trick" discourage good design? It observes encapsulation restrictions. If you publish a private method, other classes may depend on implementation details.

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

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