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

Beat count to next cue #4786

Closed
wants to merge 39 commits into from

Conversation

jmmaldonado
Copy link

Adds a beat counter from the current play position to the nearest cue point
The information is displayed in the deck time widget
Created a new option when selecting the time display for the decks so you can choose to show the information or keep the traditional deck time information.

@github-actions github-actions bot added the ui label Jun 5, 2022
@JoergAtGithub
Copy link
Member

This works for me, and it is a helpful addition!
Some things I noticed:

  • The option in the preferences is always "Elapsed", after reopening the Preference dialog.
  • If the loop is placed before the next cue point, it counts down until this loop (is this bug or feature?)
  • After the last cue point, just "beats" is shown (wouldn't it help to count beats until track end?)

@jmmaldonado
Copy link
Author

jmmaldonado commented Jun 5, 2022

Thanks for the feedback Joerg ! Glad you find it useful too :)

  • I didn't notice that, it is true I didn't re-open the preferences dialog right away but rather after closing and re-launching mixxx, in this case the setting persisted, but will take a look at the preferences dialog
  • I would say bug, definitely not what I intended, I would need to filter down on the specific Cue point type ... unless you find this useful, happy to discuss further :)
  • I added a different text message, perhaps I forgot to push it, but you are right ... instead of "no cues left", the countdown to the outro would be better

Thanks for taking a look !

@Be-ing
Copy link
Contributor

Be-ing commented Jun 5, 2022

Please post screenshots when you modify the GUI

@jmmaldonado
Copy link
Author

Preferences dialog
preferences1

@jmmaldonado
Copy link
Author

Beat counter UI
beat counter 1

@daschuer
Copy link
Member

daschuer commented Jun 5, 2022

Thank you, I like the idea.

Mixxx uses ControlObjects to communicate between different parts and the GUI or controller scrips.

This is also done for the "time_remaining"

m_pTimeRemaining = new ControlProxy(

To follow this pattern, I suggest to create a new ControlObject for your purpose. That can be written in DeckVisuals::process() for instance.

@JoergAtGithub
Copy link
Member

To fix the Pre-Commit issue, you can download the file pre-commit.patch from the Github artifacts, unzip it in your Mixxx directory, and call git apply pre-commit.patch
This works also on Windows.

@jmmaldonado
Copy link
Author

Thank you everyone for your help here, first time contributing so lots of room for learning and improving !
I will take a look to all the feedback provided and move to a ControlObject implementation as soon as I can

In terms of the UI, having the beat count in the track time widget was more like a proof of concept than anything else, I think I would rather have that counter directly over the play head in the waveform, but this may be too much of a UI change. WDYT ?

@Be-ing
Copy link
Contributor

Be-ing commented Jun 7, 2022

Have you considered putting this information in the text that appears when hovering cue points on the overview waveform instead? Are you sure you'd want to see this all the time? I'd imagine it would get kinda annoying when you don't have a bunch of cues on a track.

@jmmaldonado
Copy link
Author

Have you considered putting this information in the text that appears when hovering cue points on the overview waveform instead? Are you sure you'd want to see this all the time? I'd imagine it would get kinda annoying when you don't have a bunch of cues on a track.

To my style of mixing having this information always visible and at hand is essential, maybe having it on the waveform could be overkill for most people but if we go that route there should be a preferences option to show / hide that info.
In the meantime I believe having the beat counter in the track time widget is not very intrusive and as it is right now you can always fall back to the traditional elapsed / remaining / elapsed + remaining setting

What I was thinking about is something like rekordbox displays on top of the waveform, I use it extensively in my sets
image_2022-06-08_200338996

@JoergAtGithub
Copy link
Member

For me, the feature makes only sense, if it's always visible.
I recommend to keep this initial PR simple, you can improve it in follow-up PRs.

@jmmaldonado
Copy link
Author

Makes sense, I will make sure the PR works with the current way of getting the play position (and it passes all the tests!) and then iterate to move to COs and start improving from there. Thanks !

The value was saved correctly but the new radio button was never checked once reading the preferences
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.

Just some findings in the pref ui file.

src/preferences/dialog/dlgprefdeckdlg.ui Outdated Show resolved Hide resolved
@jmmaldonado jmmaldonado requested a review from ronso0 June 18, 2022 15:33
@jmmaldonado
Copy link
Author

I think I finally got clang-format working on windows, so hopefully no more code style errors
Trying now to get clazy to work as well, but at least I fixed the errors shown in the last build
Apologies for all the coming back & forth with this

@ronso0
Copy link
Member

ronso0 commented Jun 21, 2022

@ronso0
Copy link
Member

ronso0 commented Jun 21, 2022

To the see the beats countdown I'd need to select ".. Remaining", though I'd appreciate if the the beat countdown would be a independent option (checkbox below the time display row).
What do others think?

@jmmaldonado
Copy link
Author

jmmaldonado commented Jun 25, 2022

Please check the CI tests: There is a typo https://github.com/mixxxdj/mixxx/runs/6975409044?check_suite_focus=true#step:5:92 and some clazy error https://github.com/mixxxdj/mixxx/runs/6975409048?check_suite_focus=true#step:9:486

Will take a look right away, thanks !

To the see the beats countdown I'd need to select ".. Remaining", though I'd appreciate if the the beat countdown would be a independent option (checkbox below the time display row). What do others think?

Re-reading the comment I see what you mean, so a separate option to decide whether to show it or not, but then keep it displayed on the track time counters, right?
in any case, for the future of the feature I think I will branch from what we have today and try to display it on top of the waveform as other software do, with a checkbox to select if we want it on display or not. In terms of feature design is at least what I would expect, and more convenient to sync the beat counters between tracks (less eye travel time to ensure they are in sync)

@ronso0
Copy link
Member

ronso0 commented Jun 26, 2022

Re-reading the comment I see what you mean, so a separate option to decide whether to show it or not, but then keep it displayed on the track time counters, right?

Jep, that's what I mean.
Maybe the pos display widget (max) size needs to be adjusted in skins.

display it on top of the waveform as other software do, with a checkbox to select if we want it on display or not.

Sounds good.
Please check with more than on skin when you test it. Skins show all sorts of markers in different positions (cue, hotcue, loops, intro, outro : left|right, top|center|bottom), and there will hopefully be a way to display the [remaining beats] without obscurring other potentially more relevant information.

@jmmaldonado
Copy link
Author

Re-reading the comment I see what you mean, so a separate option to decide whether to show it or not, but then keep it displayed on the track time counters, right?

Jep, that's what I mean. Maybe the pos display widget (max) size needs to be adjusted in skins.

I will finish the feature with the comments you made about having the option as an independent checkbox so we have it ready to merge if we want to, and then move to the advanced visualization on the waveform. As you said, it will probably take us more iterations to decide whether it is the right place to show or not

@github-actions
Copy link

github-actions bot commented Oct 7, 2022

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 7, 2022
@tippfehlr
Copy link

Not exactly this PR but I would find it helpful to display the beat count along with the time on intro/outro sections
Screenshot_20230317_175510

@JoergAtGithub
Copy link
Member

Not exactly this PR but I would find it helpful to display the beat count along with the time on intro/outro sections Screenshot_20230317_175510

Make sense to me

@ronso0 ronso0 linked an issue Aug 14, 2023 that may be closed by this pull request
@ejgutierrez74
Copy link

When we are going to see this feature perhaps optional ( preference settings):

imagen

I suppose it would come with 2.3.6 ? Perhaps in 2.4 ? Or it would never came to mixxx ?

Thanks

@ronso0
Copy link
Member

ronso0 commented Aug 15, 2023

2.3.6 was released today, 2.4 will come during the next few weeks.

If someone finishes / adopts this stale PR there's a foundation for other display options.

@ejgutierrez74
Copy link

2.3.6 was released today, 2.4 will come during the next few weeks.

If someone finishes / adopts this stale PR there's a foundation for other display options.

But you have to approve the changes
imagen

In history seems that jmaldonado made the changes you ask for a year ago or so....

Seems not very difficult to implement this.

Thanks for your answer

@ronso0
Copy link
Member

ronso0 commented Aug 16, 2023

Nope, the separate preference checkbox has not been implemented, yet.

Re-reading the comment I see what you mean, so a separate option to decide whether to show it or not, but then keep it displayed on the track time counters, right?

2.4 feature freeze has long passed, so this will go to 2.5 soonest.

@jmmaldonado Do you intend (and have time) to finish this?

@jmmaldonado
Copy link
Author

jmmaldonado commented Aug 19, 2023

I definitely want to get back to it - and get the dev env up again, had to tear it down - no ETA I can commit to though :(

In the meantime @ejgutierrez74 if you want to try it as it is right now with the basic functionality working but not very well integrated in the UI, you can build from the fork in my repo and see how it goes, we could use your comments and help with the implementation if you are up for it too

@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Aug 25, 2023
@mxmilkiib
Copy link
Contributor

This was superseded by #12994, yes?

@ronso0
Copy link
Member

ronso0 commented May 26, 2024

I think so, yes.
Closing this to clean up the PR list.
Feel free to adopt / reopen.

@ronso0 ronso0 closed this May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

show beats until next hot cue
8 participants