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

Fixes newly added tracks in BB having wrong size #2883

Merged
merged 1 commit into from Jul 5, 2016

Conversation

Projects
None yet
5 participants
@Stephen-Seo
Contributor

Stephen-Seo commented Jun 30, 2016

This PR is technically a continuation of #2880 , with better fixes made as suggested by @jasp00 .

This bug is mentioned in issue #2476 .

Show outdated Hide outdated src/tracks/Pattern.cpp
@@ -125,6 +125,55 @@ void Pattern::init()
changeLength( length() );
restoreJournallingState();

This comment has been minimized.

@jasp00

jasp00 Jul 1, 2016

Member

init() is also run for the copy constructor, which is correctly initialized. This code should only run for the other constructor and before ensureBeatNotes(), thus before init().

@jasp00

jasp00 Jul 1, 2016

Member

init() is also run for the copy constructor, which is correctly initialized. This code should only run for the other constructor and before ensureBeatNotes(), thus before init().

Show outdated Hide outdated src/tracks/Pattern.cpp
{
// Do this per BB for the current Track
for(unsigned int tcoIndex = 0; tcoIndex < m_instrumentTrack->getTCOs().size();
++tcoIndex)

This comment has been minimized.

@jasp00

jasp00 Jul 1, 2016

Member

This should be done only for the TCO the pattern belongs to. You can get the tcoIndex with getTCOs().indexOf(this).

@jasp00

jasp00 Jul 1, 2016

Member

This should be done only for the TCO the pattern belongs to. You can get the tcoIndex with getTCOs().indexOf(this).

Show outdated Hide outdated src/tracks/Pattern.cpp
// Get one other track in the currentTrackContainer
for(unsigned int trackIndex = 0;
trackIndex < currentTrackContainer->tracks().size();
++trackIndex)

This comment has been minimized.

@jasp00

jasp00 Jul 1, 2016

Member

Track 0 is supposed to have good patterns. New tracks are appended to the end.

@jasp00

jasp00 Jul 1, 2016

Member

Track 0 is supposed to have good patterns. New tracks are appended to the end.

This comment has been minimized.

@jasp00

jasp00 Jul 1, 2016

Member

Rather than track 0, the first track that is an instrument track.

@jasp00

jasp00 Jul 1, 2016

Member

Rather than track 0, the first track that is an instrument track.

Show outdated Hide outdated src/tracks/Pattern.cpp
continue;
}
Pattern * pattern = dynamic_cast<Pattern *>(currentTrackContainer->tracks().at(trackIndex)->getTCO(tcoIndex));

This comment has been minimized.

@jasp00

jasp00 Jul 1, 2016

Member

You may use a static_cast if the track is an instrument track.

@jasp00

jasp00 Jul 1, 2016

Member

You may use a static_cast if the track is an instrument track.

Show outdated Hide outdated src/tracks/Pattern.cpp
Pattern * currentPattern = dynamic_cast<Pattern *>(m_instrumentTrack->getTCO(tcoIndex));
currentPattern->m_steps = steps;

This comment has been minimized.

@jasp00

jasp00 Jul 1, 2016

Member

In the end, only one pattern should be fixed, the one running the code.

@jasp00

jasp00 Jul 1, 2016

Member

In the end, only one pattern should be fixed, the one running the code.

@Stephen-Seo

This comment has been minimized.

Show comment
Hide comment
@Stephen-Seo

Stephen-Seo Jul 1, 2016

Contributor

OK, I've taken the notices from jasp00 and made a fix that should be correct in execution and design. Hopefully this will be the last revision but I am still open to notices and flaws about the code. Many thanks, jasp00!

Contributor

Stephen-Seo commented Jul 1, 2016

OK, I've taken the notices from jasp00 and made a fix that should be correct in execution and design. Hopefully this will be the last revision but I am still open to notices and flaws about the code. Many thanks, jasp00!

Show outdated Hide outdated src/tracks/Pattern.cpp
// Resize this track to be the same as existing tracks in the BB
// Only if there already exists at least another track in the BB
const TrackContainer::TrackList & tracks = m_instrumentTrack->trackContainer()->tracks();

This comment has been minimized.

@jasp00

jasp00 Jul 2, 2016

Member

Try to limit line lengths to 80 characters (coding conventions).

@jasp00

jasp00 Jul 2, 2016

Member

Try to limit line lengths to 80 characters (coding conventions).

Show outdated Hide outdated src/tracks/Pattern.cpp
// Resize this track to be the same as existing tracks in the BB
// Only if there already exists at least another track in the BB
const TrackContainer::TrackList & tracks = m_instrumentTrack->trackContainer()->tracks();
if(tracks.size() > 1)

This comment has been minimized.

@jasp00

jasp00 Jul 2, 2016

Member

You should remove this check. It will make the algorithm simpler and faster for normal projects.

@jasp00

jasp00 Jul 2, 2016

Member

You should remove this check. It will make the algorithm simpler and faster for normal projects.

Show outdated Hide outdated src/tracks/Pattern.cpp
for(; trackID < tracks.size(); ++trackID)
{
// skip track that is the same as the current Pattern
if(tracks.at(trackID) == static_cast<Track *>(m_instrumentTrack))

This comment has been minimized.

@jasp00

jasp00 Jul 2, 2016

Member

When you hit this condition, there are no more instrument tracks. You should simply check that the track is an instrument track; then if the track is different from m_instrumentTrack, m_steps is updated.

@jasp00

jasp00 Jul 2, 2016

Member

When you hit this condition, there are no more instrument tracks. You should simply check that the track is an instrument track; then if the track is different from m_instrumentTrack, m_steps is updated.

Show outdated Hide outdated src/tracks/Pattern.cpp
}
// Found existing InstrumentTrack, thus break
else if(tracks.at(trackID)->type() == Track::InstrumentTrack)
{

This comment has been minimized.

@jasp00

jasp00 Jul 2, 2016

Member

If you update m_steps here, it is true that trackID < tracks.size().

@jasp00

jasp00 Jul 2, 2016

Member

If you update m_steps here, it is true that trackID < tracks.size().

Show outdated Hide outdated src/tracks/Pattern.cpp
// If trackID is out of bounds, then there is no valid InstrumentTrack
// If currentTCO is out of bounds, then a new BB was created and should
// have default size
if(trackID < tracks.size() && currentTCO < tracks.at(trackID)->numOfTCOs())

This comment has been minimized.

@jasp00

jasp00 Jul 2, 2016

Member

currentTCO will be in bounds since the reference track comes before and is already initialized.

@jasp00

jasp00 Jul 2, 2016

Member

currentTCO will be in bounds since the reference track comes before and is already initialized.

Show outdated Hide outdated src/tracks/Pattern.cpp
// If currentTCO is out of bounds, then a new BB was created and should
// have default size
if(trackID < tracks.size() && currentTCO < tracks.at(trackID)->numOfTCOs())
{

This comment has been minimized.

@jasp00

jasp00 Jul 2, 2016

Member

Then, currentTCO's initialization should be here.

@jasp00

jasp00 Jul 2, 2016

Member

Then, currentTCO's initialization should be here.

@jasp00

This comment has been minimized.

Show comment
Hide comment
@jasp00

jasp00 Jul 2, 2016

Member

Do not spend too much time with commit messages because you should squash commit history later.

Member

jasp00 commented Jul 2, 2016

Do not spend too much time with commit messages because you should squash commit history later.

@Stephen-Seo

This comment has been minimized.

Show comment
Hide comment
@Stephen-Seo

Stephen-Seo Jul 2, 2016

Contributor

The reason why I check if "currentTCO" is in bounds, is that when another BB is added, the TCO for that BB doesn't yet exist for the tracks during execution of Pattern's constructor. Since the newly added BB will always have the default length, it is better to check if the "currentTCO" is in bounds and not change "m_steps" if it isn't.

Contributor

Stephen-Seo commented Jul 2, 2016

The reason why I check if "currentTCO" is in bounds, is that when another BB is added, the TCO for that BB doesn't yet exist for the tracks during execution of Pattern's constructor. Since the newly added BB will always have the default length, it is better to check if the "currentTCO" is in bounds and not change "m_steps" if it isn't.

@Stephen-Seo

This comment has been minimized.

Show comment
Hide comment
@Stephen-Seo

Stephen-Seo Jul 2, 2016

Contributor

If I ignore bounds checking for "currentTCO", I get a "called Track::getTCO( 3 ), but TCO 3 doesn't exist"

Contributor

Stephen-Seo commented Jul 2, 2016

If I ignore bounds checking for "currentTCO", I get a "called Track::getTCO( 3 ), but TCO 3 doesn't exist"

@Stephen-Seo

This comment has been minimized.

Show comment
Hide comment
@Stephen-Seo

Stephen-Seo Jul 2, 2016

Contributor

If you are satisfied with the current changes, then I will sqash commit history, but until then I will keep on refactoring.

Contributor

Stephen-Seo commented Jul 2, 2016

If you are satisfied with the current changes, then I will sqash commit history, but until then I will keep on refactoring.

Show outdated Hide outdated src/tracks/Pattern.cpp
m_instrumentTrack->trackContainer()->tracks();
for(unsigned int trackID = 0; trackID < tracks.size(); ++trackID)
{
if(tracks.at(trackID)->type() == Track::InstrumentTrack &&

This comment has been minimized.

@jasp00

jasp00 Jul 2, 2016

Member

You should break on this condition. The reference track should be before the current track, not after. Then, you can remove the bounds check for currentTCO. It should look like:

if(type)
  if(not same track)
    update steps
  break
@jasp00

jasp00 Jul 2, 2016

Member

You should break on this condition. The reference track should be before the current track, not after. Then, you can remove the bounds check for currentTCO. It should look like:

if(type)
  if(not same track)
    update steps
  break
Show outdated Hide outdated src/tracks/Pattern.cpp
if(tracks.at(trackID)->type() == Track::InstrumentTrack &&
tracks.at(trackID) != m_instrumentTrack)
{
int currentTCO = m_instrumentTrack->getTCOs().indexOf(this);

This comment has been minimized.

@jasp00

jasp00 Jul 2, 2016

Member

When limiting the line to 80 characters, remember that one tab equals to 8 spaces.

@jasp00

jasp00 Jul 2, 2016

Member

When limiting the line to 80 characters, remember that one tab equals to 8 spaces.

Show outdated Hide outdated src/tracks/Pattern.cpp
break;
}
m_steps = static_cast<Pattern *>
(tracks.at(trackID)->getTCO(currentTCO))->m_steps;

This comment has been minimized.

@jasp00

jasp00 Jul 2, 2016

Member

80 characters.

@jasp00

jasp00 Jul 2, 2016

Member

80 characters.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 2, 2016

Member

Let's please stop with the 80 character requirement. It just makes the code ugly. I realize the wiki offers it as a recommendation, but there's very little benefit in enforcing it anymore.

Member

tresf commented Jul 2, 2016

Let's please stop with the 80 character requirement. It just makes the code ugly. I realize the wiki offers it as a recommendation, but there's very little benefit in enforcing it anymore.

@Stephen-Seo

This comment has been minimized.

Show comment
Hide comment
@Stephen-Seo

Stephen-Seo Jul 3, 2016

Contributor

I usually had used "set expandtab", "set tabstop=4" and "set shiftwidth=4" in my vim config.
I knew to disable "set expandtab" to avoid mistabbing things but didn't realize that they count for 8 spaces. The fix should be good now?

Also, is it safe to squelch commits into one commit within the bb_bugfix branch or will I need to do it in a new branch and a new PR? (I won't be doing it until it's truely fixed.)

Contributor

Stephen-Seo commented Jul 3, 2016

I usually had used "set expandtab", "set tabstop=4" and "set shiftwidth=4" in my vim config.
I knew to disable "set expandtab" to avoid mistabbing things but didn't realize that they count for 8 spaces. The fix should be good now?

Also, is it safe to squelch commits into one commit within the bb_bugfix branch or will I need to do it in a new branch and a new PR? (I won't be doing it until it's truely fixed.)

@jasp00

This comment has been minimized.

Show comment
Hide comment
@jasp00

jasp00 Jul 3, 2016

Member

The fix is good. You can squash commits within the bb_bugfix branch. Try git rebase -i HEAD~10 (10 commits).

Member

jasp00 commented Jul 3, 2016

The fix is good. You can squash commits within the bb_bugfix branch. Try git rebase -i HEAD~10 (10 commits).

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 3, 2016

Member

The fix is good. You can squash commits within the bb_bugfix branch. Try git rebase -i HEAD~10 (10 commits).

FYI, @jasp00 GitHub provides a "Squash and merge" button now.

screen shot 2016-07-03 at 11 36 30 am

Member

tresf commented Jul 3, 2016

The fix is good. You can squash commits within the bb_bugfix branch. Try git rebase -i HEAD~10 (10 commits).

FYI, @jasp00 GitHub provides a "Squash and merge" button now.

screen shot 2016-07-03 at 11 36 30 am

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 5, 2016

Member

Tested this out, works beautifully.
The only thing is that the new track is always the same size as the first track. Now, you can edit out the first track's length using the context menu, and every new track would have that new length, despite all the other tracks having a different length.

But I don't see this is as an issue, and also making the new track adding system more "intelligent" probably isn't worth it, and in most cases this fix works like a charm. So this gets a 👍 from me for a merge.

Member

Umcaruje commented Jul 5, 2016

Tested this out, works beautifully.
The only thing is that the new track is always the same size as the first track. Now, you can edit out the first track's length using the context menu, and every new track would have that new length, despite all the other tracks having a different length.

But I don't see this is as an issue, and also making the new track adding system more "intelligent" probably isn't worth it, and in most cases this fix works like a charm. So this gets a 👍 from me for a merge.

@tresf tresf merged commit 652aa5e into LMMS:master Jul 5, 2016

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@jasp00

jasp00 Jul 6, 2016

Member

TBH @Umcaruje, I did not notice patterns could be changed individually. In this case, the default number of steps could be saved in BB tracks, as suggested by @Stephen-Seo.

Member

jasp00 commented Jul 6, 2016

TBH @Umcaruje, I did not notice patterns could be changed individually. In this case, the default number of steps could be saved in BB tracks, as suggested by @Stephen-Seo.

@Spekular

This comment has been minimized.

Show comment
Hide comment
@Spekular

Spekular Jul 6, 2016

Contributor

I don't really think it's necessary to have different lengths for different samples, so maybe the context menu action should be removed or changed to affect all samples

Contributor

Spekular commented Jul 6, 2016

I don't really think it's necessary to have different lengths for different samples, so maybe the context menu action should be removed or changed to affect all samples

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