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

This fixes Auto DJ stop after the track lengh changed #11479

Merged
merged 4 commits into from
Apr 17, 2023

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Apr 14, 2023

This likely fixes #11448
All positions that are used in Auto DJ are now clamped to the track end.
This ensures that the Auto DJ keeps going even if a faulty track is played.

This also refreshes the invisible AudibleSound cue when the user re-analyzes waveforms.
Before, the only way to reset this cue was to reset either intro or outro, which they might want to keep.

@daschuer daschuer added this to the 2.3.5 milestone Apr 14, 2023
@daschuer daschuer force-pushed the gh11448 branch 2 times, most recently from 3c8a9c5 to 7c039ad Compare April 15, 2023 23:07
@daschuer
Copy link
Member Author

I have moved the Audible Sound cue reset commit to #11482
This way this PR becomes a pure Bug-Fix.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as a bandaid fix. In the future we really need to come up with a clear way of invalidating and regenerating track-related data. Otherwise checks like these will get out of hand really quick.

@Swiftb0y Swiftb0y merged commit 3529d35 into mixxxdj:2.3 Apr 17, 2023
Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some ideas for improvement

}
}
return getEndSecond(pDeck);
return samplePositionToSeconds(endSamplePosition, pDeck);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and L1138: why not use existing return getEndSecond(pDeck);?
That way the fix is only the renaming and the extra check in the if (lastSound > 0) branch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid the overhead to fetch endSamplePosition again in getEndSecond(). It is also one less redirection for a better understanding.

Comment on lines +1146 to +1147
qWarning() << "Audible Sound Cue ends after track end using:"
<< pTrack->getLocation();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add

                           << "Returning last sample instead.";

?

@@ -1119,11 +1119,17 @@ double AutoDJProcessor::getFirstSoundSecond(DeckAttributes* pDeck) {
return 0.0;
}

double endSamplePosition = pDeck->trackSamples();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this down to where it's needed?
or even use it directly for the <= comparison?

@@ -1074,16 +1074,18 @@ void AutoDJProcessor::playerOutroEndChanged(DeckAttributes* pAttributes, double
}

double AutoDJProcessor::getIntroStartSecond(DeckAttributes* pDeck) {
double endSamplePosition = pDeck->trackSamples();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here and below:
trackEndSample for consistent naming?

@ronso0
Copy link
Member

ronso0 commented Apr 17, 2023

Whoops, too late...

@Swiftb0y
Copy link
Member

Whoops, too late...

Sorry, I'll wait a little more next time.

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

Successfully merging this pull request may close these issues.

3 participants