-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
library: display hotcues on overview waveform #15514
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
base: main
Are you sure you want to change the base?
library: display hotcues on overview waveform #15514
Conversation
- add hotcue marker rendering in library overview waveform column - load cue data and track metadata (duration, sample rate) in OverviewCache::prepareOverview() - draw hotcue markers as semi-transparent vertical lines with cue colors - calculate hotcue positions proportionally across waveform width based on track duration implements mixxxdj#14994
9323387 to
4f2aace
Compare
| markerPainter.setCompositionMode(QPainter::CompositionMode_SourceOver); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need for the intermediate baseColor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a few ways to get the lines to be as distinct as on the deck overview but struggled
10px made it almost bearable
I'll see what I can do now
and no
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a few ways to get the lines to be as distinct as on the deck overview but struggled
Okay, that's because this paints the full-width waveform and the scaling to desired width happens in OverviewCache (IIRC this has been adopted from CoverArtCache), hence no fixed width here will guarantee that the markers have adequate width in the final image in the library.
Options are a) pass the resized image here (to a new function) to paint the hotcue overlay, b) do the scaling here and paint hotcues on top, or c) ...
Back'n forth of a) is not ideal (heayv QImage), so it's b) unless you/ai figure another way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, please remove those // MARK: .. comments, IMO they're pointless
src/library/overviewcache.cpp
Outdated
| QSqlQuery query(database); | ||
| query.prepare(QStringLiteral("SELECT " LIBRARY_TABLE ".") + | ||
| LIBRARYTABLE_DURATION + QStringLiteral(", " LIBRARY_TABLE ".") + | ||
| LIBRARYTABLE_SAMPLERATE + | ||
| QStringLiteral(" FROM " LIBRARY_TABLE " WHERE " LIBRARY_TABLE ".id=:id")); | ||
| query.bindValue(":id", trackId.toVariant()); | ||
| if (query.exec() && query.next()) { | ||
| trackDurationMillis = query.value(0).toDouble() * | ||
| 1000.0; // convert seconds to milliseconds | ||
| sampleRate = query.value(1).toInt(); | ||
| if (sampleRate <= 0) { | ||
| sampleRate = 44100; // fallback if invalid | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing SQL queries here we may get values from columns in the delegate?
If possible I'd prefer that instead of acting on the db here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ completely removed database queries from prepareOverview()
✅ now passes TrackPointer from delegate
✅ gets cues via pTrack->getCueInfos() and duration via pTrack->getDuration()
| 2, | ||
| imageHeight - lowerMarkerYPos, | ||
| minuteColor); | ||
| markerPainter.setCompositionMode(QPainter::CompositionMode_SourceOver); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why set/reset the CompositionMode for each marker?
Consider using PainterScope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ now using PainterScope instead of manual setCompositionMode() calls
| // draw hotcue markers | ||
| if (!hotcues.isEmpty()) { | ||
| for (const auto& hotcue : hotcues) { | ||
| if (hotcue.positionMillis <= 0 || hotcue.positionMillis > trackDurationMillis) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why skip hotcues in the preroll?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ now allows negative positions (removed positionMillis <= 0 check)
✅ only skips hotcues beyond track end
| struct HotcueInfo { | ||
| double positionMillis; | ||
| mixxx::RgbColor::code_t colorCode; | ||
| QString label; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a struct if we already have the CueInfo class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ removed custom struct
✅ now uses existing mixxx::CueInfo class directly
✅ filters by CueType::HotCue in renderer
- use existing CueInfo instead of custom HotcueInfo struct - pass track metadata from delegate instead of SQL queries in OverviewCache - use PainterScope for hotcue rendering - match deck overview marker style: 1px border + 1px fill line - allow preroll hotcues (negative positions) - remove unnecessary intermediate variables
Track does not have a getCueInfos() method - need to iterate through getCuePoints() and call getCueInfo(sampleRate) on each cue
displays hotcue markers and minute markers on library overview waveform column, similar to rekordbox.
changes:
implementation:
fixes #14994