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

Library: add overview column #14140

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jan 8, 2025

continuation of #13638
This is now based on @ninomp's initial POC commit 80d8b5f
from https://github.com/ninomp/mixxx/commits/overviewsinlibrary-rebased/
(incl. the fix from #14150 )
I addressed @Swiftb0y's review and a lot evolved anyway.

image

  • the library shows a pseudo-mono view (hardcoded in OverviewCache::prepareOverview), though with a bit of refactoring this could be made optional
  • overviews are rendered as soon as track analysis finished (loading tracks not required)
  • overviews (decks and library) are updated when the library row height, overview type or the Normalize/visual gain options are changed, and the cache is cleared
  • the signal colors can be set in the <Library> node exactly like in the waveform templates (<Visual> widget)
  • WOverView and OverviewDelegate use the same renderer for all types which I extracted from WOverview to src/waveform/renderers/waveformoverviewrenderer
    (I adopted the RGB simplifications made by @Nino MP, and simplified the others, too)
  • there's an OverviewCache which holds the images sized for the overview column

Ideas / Nice to have

  • show analyzer progress / partial painting like in decks
  • add mono-mixdown / stereo option
  • click Overview column header to sort by 'has overview'
  • cleanup / optimize draw functions in WaveformOverviewRenderer
  • some sort of analysis progress indicator in the library? (though I think partial rendering would be distracting)
  • implement preview interface: click to select, click position to preview
  • show cue markers?

@ronso0
Copy link
Member Author

ronso0 commented Jan 8, 2025

Draft until CI is happy.
I'll rebase and force-push, so please don't start reviewing yet.

@ronso0 ronso0 force-pushed the lib-overview-column-ro branch 2 times, most recently from ec7c78c to b0038fb Compare January 8, 2025 13:48
@ronso0 ronso0 force-pushed the lib-overview-column-ro branch from b0038fb to fba0ecc Compare January 9, 2025 02:00
@ronso0 ronso0 marked this pull request as ready for review January 9, 2025 02:00
@ronso0
Copy link
Member Author

ronso0 commented Jan 9, 2025

I took a second look and polished it a bit.
Ready for review!

@ronso0 ronso0 force-pushed the lib-overview-column-ro branch from 17243c3 to d644456 Compare January 9, 2025 13:39
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Left some comments, not tested yet.

&TrackAnalysisScheduler::trackProgress,
this,
&AnalysisFeature::trackProgress,
Qt::DirectConnection);
Copy link
Member

Choose a reason for hiding this comment

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

We should not use a direct connection here, this may cause headaches later. See my other comment.

@acolombier
Copy link
Member

@ronso0 I'm marking this PR as draft, but feel free to make active once everything is ready for another round of tests!

@acolombier acolombier marked this pull request as draft February 18, 2025 01:43
@ronso0
Copy link
Member Author

ronso0 commented Feb 18, 2025

Sure, I'm in no hurry here, and apparently there's not too much interest.
This has been ready for testing for a few weeks already and the technical reviews have been adressed in main already, too.
Will merge main soon.

@acolombier
Copy link
Member

acolombier commented Feb 18, 2025

there's not too much interest

I'd love to see this feature in! But equally spotted a lot of reviewer involved with this feature, and the few PR iteration so I didn't want to create a "too many cooks" problem.

This has been ready for testing for a few weeks already

Will test soon then

@fwcd
Copy link
Member

fwcd commented Feb 19, 2025

Very interested in seeing this too! Gave the macOS build a quick spin and couldn't find any obvious performance regressions, seems to run pretty snappily. Interestingly, this seems to be mostly unaffected from #14089.

@acolombier
Copy link
Member

Tested on Linux (Fedora/Wayland) and works great! No performance impact, run smoothly. Only thing that would be great to change is to remove the sorting capability on the column as it has no effect (and probably shouldn't have any), but that's minor and happy to merge without this.
image

@fwcd
Copy link
Member

fwcd commented Feb 19, 2025

I actually stumbled upon this and had a funny idea: What if sorting the overview column would sort by average energy or something? Would probably be terribly slow to compute given that waveforms are loaded lazily and introducing something like that would probably be better modeled as a separate analysis/column (#12131) 😄

@ronso0
Copy link
Member Author

ronso0 commented Feb 19, 2025

and apparently there's not too much interest.

Okay okay so I was wrong 😄
Thanks for testing!
Yes it runs pretty smooth

Credits go to @ninomp who did most of the work here!

@ywwg
Copy link
Member

ywwg commented Feb 20, 2025

I love this feature and have been running it in my personal builds! really interested in this getting in

@ronso0
Copy link
Member Author

ronso0 commented Feb 20, 2025

Alright then, I'll squash and rebase, r just squash and merge main?
What's preferred here?

(I think the last remaining review comment (Qt::DirectConnection is obsolete after merge/rebase)

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.

6 participants