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

show beats and time until next marker #12994

Merged
merged 30 commits into from
May 6, 2024
Merged

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented Mar 23, 2024

Show the number of beat and/or time until the next marker, inside the waveform.

also_show_time.mov

The digits are rendered using a texture (generated on the fly), for two reasons:

  • adding a dark blurred outline so the text is readable even when displayed on top of the waveform (similar to tv subtitles).
  • efficiency

The display can be configured in the settings:

E.g:
Screenshot 2024-04-07 at 18 52 02

Looks like this:
Screenshot 2024-04-07 at 18 52 25

@m0dB m0dB marked this pull request as draft March 23, 2024 15:33
@m0dB m0dB changed the title show beats until next marker [DRAFT] show beats until next marker Mar 23, 2024
@m0dB m0dB force-pushed the beats-until-marker branch 2 times, most recently from d1c1b4b to 95879fb Compare March 25, 2024 08:25
@github-actions github-actions bot added the build label Mar 25, 2024
@m0dB
Copy link
Contributor Author

m0dB commented Mar 25, 2024

I pushed a wrong commit. correct now.

@fwcd
Copy link
Member

fwcd commented Mar 26, 2024

Neat, will definitely have to play around with this PR once I find some time. Just curious, how does this handle markers that are slightly off, i.e. not sample-perfectly aligned to the grid? (My library has quite a few of them since I tend to have quantization off) Are the beats rounded or floored?

Would be awesome if this were configurable too, e.g. whether to show beats and/or time, perhaps even the size/opacity. I could imagine the UI getting a bit noisy with too much text (especially with the time).

@m0dB
Copy link
Contributor Author

m0dB commented Mar 26, 2024

That's a good question! I only have my markers on the beat 😄

It uses Beats::ConstIterator Beats::iteratorFrom(audio::FramePos position), which does std::lower_bound, so floor. I think this makes sense also conceptually for markers that are not on the beat. After all, it's until the beat that contains the marker.

@ywwg
Copy link
Member

ywwg commented Mar 26, 2024

I have been thinking about exactly this sort of UX addition, thank you for working on it. In my own head, I was thinking about putting this information in the overview rather than the waveform itself. Right now if the mouse is over the next marker, I get a bunch of useful information, including time left until that marker. What if we always showed that information for the next marker in the track? And then the user could choose between beat counts and time counts. That would keep the big waveform clear of numbers.

As written, from a UX perspective, I would prefer the numbers to be at the top of the play marker, not right in the middle where it visually crowds the waveform display.

@fwcd
Copy link
Member

fwcd commented Mar 26, 2024

I think this makes sense also conceptually for markers that are not on the beat. After all, it's until the beat that contains the marker.

Maybe we could count full beats relative to the next marker? That would take care of the issue, regardless of how misaligned they are. Though that might become challenging with variable grids.

By the way, on an unrelated note, being able to turn quantization on only for setting markers (but not start/stop or jumping to them) would be really awesome.

@ywwg
Copy link
Member

ywwg commented Mar 26, 2024

I think we could round the beat count if the marker is not exactly on the beat.

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.

Thank you. Some comment.

src/waveform/renderers/allshader/digitsrenderer.cpp Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member

That looks really nice, thanks. I also love that you seem to have the habit of adding Screencast to your PRs so that it's easy to see the result w/o checking out and building the changes.

One easy improvement I can think of when looking at the recordings is to not center the information vertically. It's a bit hard to read with the signal in the background, so I'd suggest to align them them at the top where the bg is black (at the bottom it would conflict with hotcue labels).

@m0dB
Copy link
Contributor Author

m0dB commented Apr 7, 2024

I have added options in the settings dialog to configure what is displayed (none, beats, time, beats + time, beats + time (multiline)), where it is displayed (top, center, bottom), and the font size.

@ywwg
Copy link
Member

ywwg commented Apr 9, 2024

do you have more you want to do before marking this as ready for review? I think in general people like the feature idea!

@m0dB
Copy link
Contributor Author

m0dB commented Apr 19, 2024

do you have more you want to do before marking this as ready for review? I think in general people like the feature idea!

no, I think it is ready for review, once I resolve the conflicts with main.

@m0dB m0dB requested a review from daschuer April 20, 2024 00:09
@m0dB m0dB marked this pull request as ready for review April 20, 2024 00:09
@m0dB m0dB changed the title [DRAFT] show beats until next marker show beats and time until next marker Apr 20, 2024
@m0dB
Copy link
Contributor Author

m0dB commented Apr 20, 2024

This is now ready for review.

Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

The preferences option appear for all waveform types, but the feature works only for the allshaders GLSL waveforms. Either disable the settings for the unsupported waveform types, or add support for all waveform types.

@JoergAtGithub
Copy link
Member

Is it the intended behavior, that loop end is considered as marker?

@m0dB
Copy link
Contributor Author

m0dB commented Apr 20, 2024

The preferences option appear for all waveform types, but the feature works only for the allshaders GLSL waveforms. Either disable the settings for the unsupported waveform types, or add support for all waveform types.

I am definitely not going to add support for all waveform types, but I will disable them and add a "Requires GLSL waveform type" label. Thanks for spotting this, I was planning to do this but forgot about it.

EDIT: DONE!

@m0dB
Copy link
Contributor Author

m0dB commented May 6, 2024

rebased on main (had to resolve some conflicts with @acolombier 's slipmode code) and added @daschuer static_assert suggestion.

@m0dB
Copy link
Contributor Author

m0dB commented May 6, 2024

@daschuer wrote

Thank you for this nice feature. And thank you for the extra efforts polishing the code.

You are welcome. And the suggestions, both for code quality and for feature improvements, were much appreciated.

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.

OK, than lets integrate it!

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.

7 participants