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

This makes beatmap behaves like a beatgrid when constant tempo preference is true #1

Merged
merged 3 commits into from
May 13, 2020
Merged

This makes beatmap behaves like a beatgrid when constant tempo preference is true #1

merged 3 commits into from
May 13, 2020

Conversation

crisclacerda
Copy link

This makes beatmap behaves like a beatgrid when constant tempo preference is true and also fix scons build.

@JaviVilarroig
Copy link
Owner

Hi Cristiano.
Sorry I didn't see that until now. I'm not used to github interface :S
I don't use Scons any more so I'm not going to test the fix, but it looks simple and right :)
About the constant tempo part. I'm not sure if it's a good solution. It's It just fixes the get getBPM part of the issue but the beats are left exactly as they are so there is a potential disagreement between the tempo shown in the screen and how the beats are painted in the waveform.
Have you think on that? Probably the opinion from the developers team can have a value.

@crisclacerda
Copy link
Author

crisclacerda commented May 6, 2020

No worries.
Yes, that's something @Be-ing have posted on Zulip:

"For a roadmap for mixxxdj#2512, I propose to use the existing "assume constant tempo" preference option to generate a constant tempo beatgrid functionally equivalent to the old BeatGrid class but using the new Beats API. This will allow us to merge it to master sooner."

That's what I have tried to implement here. I have tested this on a couple of tracks and it worked nice, some tracks that were problematic on the old beat grid got them painted right now.

The problem with the beatmap is that without fixing the tempo, some tracks that should have constant tempo actually display the tempo with small fluctuations while the tracks play, I think we should in fact just ignore this fluctuation and be seems to agree on that.

But one thing that I haven't put much thought here is on converting the old beat grid into the new beat map format. Especially tracks that were analyzed with the fast option and do not have all beat positions will not have beat grid painted for the whole track.

Please give this a spin and see if it works nicely for you too and feel free to just ignore and close this PR, if you don't like how it behaves.
Also, I haven't had the time to test your new patches on mixxxdj#2512 yet.

@JaviVilarroig
Copy link
Owner

No worries.
Yes, that's something @Be-ing have posted on Zulip:

"For a roadmap for mixxxdj#2512, I propose to use the existing "assume constant tempo" preference option to generate a constant tempo beatgrid functionally equivalent to the old BeatGrid class but using the new Beats API. This will allow us to merge it to master sooner."

That's what I have tried to implement here. I have tested this on a couple of tracks and it worked nice, some tracks that were problematic on the old beat grid got them painted right now.

The problem with the beatmap is that without fixing the tempo, some tracks that should have constant tempo actually display the tempo with small fluctuations while the tracks play, I think we should in fact just ignore this fluctuation and be seems to agree on that.

But one thing that I haven't put much thought here is on converting the old beat grid into the new beat map format. Especially tracks that were analyzed with the fast option and do not have all beat positions will not have beat grid painted for the whole track.

Please give this a spin and see if it works nicely for you too and feel free to just ignore and close this PR, if you don't like how it behaves.
Also, I haven't had the time to test your new patches on mixxxdj#2512 yet.

Ok. Let's use this fix to see if it's enough to solve the problem on constant tempo tracks.
Can you please take a look at the conflict? When fixed I will merge it.

@JaviVilarroig
Copy link
Owner

I wanted to use your code so finally I did the conflict resolution.

@JaviVilarroig JaviVilarroig merged commit 920d1c6 into JaviVilarroig:bars_phrases_and_variable_BPM_in_beats_class May 13, 2020
JaviVilarroig added a commit that referenced this pull request May 17, 2020
…o preference is true (#1)"

This reverts commit 920d1c6.

It introduces an issue in mixxx engine. We can continue working on this
functionality in parallel.
JaviVilarroig pushed a commit that referenced this pull request Jan 21, 2023
Shade: Put all VU meters in a fixed size parent widget
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants