Code review comment for lp:~mixxxdevelopers/mixxx/features_cuefile

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Awesome -- looks good for merging. I found these two issues:

1) TrackInfoObject::getId() only works for Mixxx library tracks. It won't work for tracks loaded from Rhythmbox, iTunes, Traktor, or Browse tracks, since all their IDs are -1. This might change in the future, but the best way is to probably just compare the TrackPointers directly. You're guaranteed that for tracks in the Mixxx library, there is only one TrackInfoObject* that exists for it, so QSharedPointer::operator= will work to tell if it's the same track. RB, iTunes, Traktor and Browse don't work this way, but at least they could be made to work that way when/if they ever keep track of the TIO's that they create. I would change all uses of getId() to compare the TrackPointers directly.

2) I tried out the cuefile recording and it didn't pick up 2 out of the 4 tracks I played. I think the problem is the math in PlayerInfo::getCurrentPlayingDeck. All the volume values are integers, while the engine volume values are 0.0 to 1.0 and the crossfader is -1.0 to 1.0, so these will get rounded off in integer conversion. Also, I think the xfvol calculation is slightly off since the orientation controls have non-intuitive values: left is 0, center is 1, right is 2.

This value for the xfval calculation worked for me (all 6 tracks I played got recorded in the correct order):

xfvol = 1 - 0.5 * fabs(m_listCOOrientation[chan]->get() - 1.0 - m_COxfader->get());

It maps tracks opposite from each other (e.g. oriented right crossfader left and vice-versa) to 0.0, and tracks that are exactly aligned w/ the crossfader (left/left, center/center, or right/right) to 1.0.

review: Approve

« Back to merge proposal