Revert "BeatGrid: Remove fuzzy behavior from findNthBeat" #4334

ywwg · 2021-09-28T14:28:56Z

This reverts commit 95f7d36.

Due to floating point inaccuracy, it's not a good idea to blindly round beat positions without some amount of fuzz. The reverted change caused https://bugs.launchpad.net/mixxx/+bug/1945238

This reverts commit 95f7d36.

Holzhaus · 2021-09-28T14:38:34Z

There must be way to fix this without doing the opposite of what the method name and documentation says (it should never be possible that findNextBeat returns a position before the search position). This is just wrong.

This rounding issue should be handled in the calling code, e. g. by calling findClosestBeat or something like that.

Holzhaus · 2021-09-28T14:40:29Z

Please add a test for the actual issue where the beatloop size is wrong.

JoergAtGithub · 2021-09-28T16:33:10Z

I struggled with the Fuzzy behaviour, while reimplementing the beat_active CO. It took me some debugging to find out, why my algorithm sometimes failed. The reason was that I got the wrong beat positions, due to this Epsilon in find prev/next beat. This was really unexpected behaviour.

Be-ing · 2021-09-28T16:40:04Z

I tested and confirm this fixes https://bugs.launchpad.net/mixxx/+bug/1942657

Be-ing · 2021-09-28T16:42:13Z

src/track/beatgrid.cpp

+    double nextBeat = ceil(beatFraction);
+
+    // If the position is within 1/100th of the next or previous beat, treat it
+    // as if it is that beat.


Please elaborate in the comment explaining this is required due to the imprecision of floating points.

Be-ing · 2021-09-28T17:23:37Z

Please amend the commit message to link https://bugs.launchpad.net/mixxx/+bug/1945238 and https://bugs.launchpad.net/mixxx/+bug/1942657

JoergAtGithub · 2021-09-28T17:27:28Z

Would this also work, if epsilon is smaller than the duration of one frame?
Than I would expect, that the issue, that the next beat is bevore the current position, will not occur anymore.

Holzhaus · 2021-09-28T17:34:05Z

The fuzzyness should not be a feature of the beatgrid, it should be handled in the calling code where it's actually needed.

Otherwise it's extremely confusing (see #3418) and we'd have to rename findNextBeat to findNextBeatExceptSometimesWhenWeJustReturnThePreviousBeatInstead.

Be-ing · 2021-09-28T17:37:12Z

If it is required everywhere I think it should be handled in the called function, not the callers.

Holzhaus · 2021-09-28T17:49:43Z

If it is required everywhere I think it should be handled in the called function, not the callers.

I doubt that it's needed everywhere.

JoergAtGithub · 2021-09-28T18:00:20Z

It's definitely not required everywhere: For findPrevNextBeats, there are two callers, which would fail with the Fuzzy behaviour enabled. Therefore this function has the additonal argumnent to enable/disable this "snap tolerance".

uklotzde · 2021-09-28T19:38:42Z

src/track/beatgrid.cpp

+
+    // If the position is within 1/100th of the next or previous beat, treat it
+    // as if it is that beat.
+    const double kEpsilon = .01;


This should be passed as a parameter instead if hardcoding it, e.g. beatPositionTolerance. A value of 0.0 should disable the fuzzy behavior. The caller knows best about the precision of the position.

A 1/100th of a frame?

@Holzhaus Other ideas?

What would be a use case for setting a different value for this?

Counter question: What is the rationale for the hard-coded constant 0.01?

I presume it was determined experimentally in the old code this is reverting to.

ywwg · 2021-09-30T01:57:01Z

This is not a new PR with new code, it is just reverting back to the old code (with a tiny fix to the test so that it compiles).
This is the traditional approach for fixing a reversion bug: reverting the bad commit first to restore the correct behavior (which was working for years). After that we can talk about better ways to fix the behavior if we want to clean it up.

Holzhaus · 2021-09-30T03:29:21Z

This is the traditional approach for fixing a reversion bug: reverting the bad commit first to restore the correct behavior (which was working for years).

Except that it doesn't restore "correct behavior" because the old code blatantly violated the spec and method documentation and sometimes did the opposite of what is expected. In that sense, this PR swaps out a bug with another (the latter is admittedly less severe because it's not user facing).

IIUC you were able to reproduce the issue with wrongly sized beatloops/beat jumps locally. This is why I asked if you could add a test for that instead of adding back a test that ensures the wrong method behavior. Then we can later revert this revert and implement another way to satisfy that test instead.

ywwg · 2021-09-30T15:46:44Z

ok I think I can construct such a test -- it might be tricky because it relies on very small floating point errors, which may not be consistent between platforms. But if I can get it to fail on one, that's a start.

ywwg · 2021-09-30T16:28:56Z

test added

daschuer · 2021-10-04T22:15:39Z

Looking at the using code, I think the issue happens in most cases where we have found a beat in first step and we just need a corresponding other beat. This is translated to a position just to fulfill the interface of the findNthBeat() function.
If we use a function that is based on a beat self the rounding cant appear at all.
I am just verifying if this works for all invocations.

ywwg · 2021-10-07T00:06:06Z

so can we at least merge the fix for the buggy behavior first?

uklotzde · 2021-10-07T10:26:44Z

Still waiting for a response to my review comments. That's what reviews are for.

ywwg · 2021-10-07T13:34:54Z

why is it my job to fix these things? @Holzhaus wrote bad code which broke mixxx, and I identified the problem and a simple remedy to get back to a working state. I'm sorry I don't have time to improve on the state of the code as it used to be. Since Jan wanted to change this code, I feel like he should be responsible for fixing his own bug. I'm really sorry I don't have more time or energy to do this but I'm not working on this any more.

ywwg · 2021-10-07T13:35:53Z

It is very hard to be in the position of being one of the only developers that seemingly uses mixxx on a regular basis. I find so many regressions and it's very hard to be asked by 5 different people to fix all of their bugs for them.

uklotzde · 2021-10-07T13:41:40Z

So you suggest us to stop working on Mixxx and leave it as is?

My review doesn't seem to be welcome.

ywwg · 2021-10-07T13:44:26Z

No, I'm asking us to follow basic software engineering process when fixing a bug: if a bad commit is found, revert the bad commit cleanly. then, after, consider the code and how it can be improved. Combining code improvements into a commit-revert is bad practice (fixing forward)

ywwg · 2021-10-07T13:46:05Z

I do a lot of work on Mixxx, I have many PRs open on which I have addressed comments. But I am out of patience for review-creep on every review.

ywwg · 2021-10-07T13:47:55Z

https://cloud.google.com/blog/products/gcp/reliable-releases-and-rollbacks-cre-life-lessons

" When an error is found or reasonably suspected in a new release, the releasing team rolls back first and investigates the problem second. A request for a rollback is not interpreted as an attack on the releasing team, or even the person who wrote the code containing the bug; rather, it is understood as The Right Thing To Do to make the system as reliable as possible for the user. No-one will ask “why did you roll back this change?” as long as the rollback changelist describes the problem that was seen."

They are talking about releases specifically, but the same thing applies to individual code commits. You start be resolving the problem. Then you take a look at improving the situation. This PR was supposed to be a rubber-stamp first step toward that goal. I do not have the time to improve every part of mixxx I find a problem with. Would you prefer I have not investigated this problem and found the bug at all? I have done my part, I would like to hand this issue off to someone else.

ywwg · 2021-10-07T13:59:11Z

My review doesn't seem to be welcome.

These are perfectly good observations and notes for whomever wants to take another stab at fixing this code. A revert is not the right place to ask for those changes.

I wish y'all could see my PRs for my job, they are short and sweet and we do what we can to get code in and make things better. I'm not asking for anything that isn't industry standard.

daschuer · 2021-10-07T14:32:08Z

I have made a good progress to fix the root cause. I think I can publish a PR today.

daschuer · 2021-10-07T17:30:50Z

Here it is: #4366

Revert "BeatGrid: Remove fuzzy behavior from findNthBeat"

e06f163

This reverts commit 95f7d36.

github-actions bot added the code quality label Sep 28, 2021

Be-ing reviewed Sep 28, 2021

View reviewed changes

Be-ing added the critical bug label Sep 28, 2021

uklotzde previously requested changes Sep 28, 2021

View reviewed changes

ywwg force-pushed the revert-fuzzy branch from 430ec1b to 2447d99 Compare September 30, 2021 21:00

Fuzzy BeatGrid: Add test that fails in the reverted code

56d291f

ywwg force-pushed the revert-fuzzy branch from 2447d99 to 56d291f Compare September 30, 2021 21:24

daschuer mentioned this pull request Oct 7, 2021

Fuzzy beats fix. #4366

Merged

ywwg closed this Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "BeatGrid: Remove fuzzy behavior from findNthBeat" #4334

Revert "BeatGrid: Remove fuzzy behavior from findNthBeat" #4334

ywwg commented Sep 28, 2021

Holzhaus commented Sep 28, 2021 •

edited

Loading

Holzhaus commented Sep 28, 2021

JoergAtGithub commented Sep 28, 2021

Be-ing commented Sep 28, 2021

Be-ing Sep 28, 2021

Be-ing commented Sep 28, 2021

JoergAtGithub commented Sep 28, 2021

Holzhaus commented Sep 28, 2021 •

edited

Loading

Be-ing commented Sep 28, 2021

Holzhaus commented Sep 28, 2021

JoergAtGithub commented Sep 28, 2021

uklotzde Sep 28, 2021

uklotzde Sep 28, 2021

uklotzde Sep 28, 2021

Be-ing Sep 28, 2021

uklotzde Sep 29, 2021

Be-ing Sep 30, 2021

ywwg commented Sep 30, 2021

Holzhaus commented Sep 30, 2021 •

edited

Loading

ywwg commented Sep 30, 2021

ywwg commented Sep 30, 2021

daschuer commented Oct 4, 2021

ywwg commented Oct 7, 2021

uklotzde commented Oct 7, 2021

ywwg commented Oct 7, 2021

ywwg commented Oct 7, 2021

uklotzde commented Oct 7, 2021

ywwg commented Oct 7, 2021

ywwg commented Oct 7, 2021

ywwg commented Oct 7, 2021 •

edited

Loading

ywwg commented Oct 7, 2021

daschuer commented Oct 7, 2021

daschuer commented Oct 7, 2021

Revert "BeatGrid: Remove fuzzy behavior from findNthBeat" #4334

Revert "BeatGrid: Remove fuzzy behavior from findNthBeat" #4334

Conversation

ywwg commented Sep 28, 2021

Holzhaus commented Sep 28, 2021 • edited Loading

Holzhaus commented Sep 28, 2021

JoergAtGithub commented Sep 28, 2021

Be-ing commented Sep 28, 2021

Be-ing Sep 28, 2021

Choose a reason for hiding this comment

Be-ing commented Sep 28, 2021

JoergAtGithub commented Sep 28, 2021

Holzhaus commented Sep 28, 2021 • edited Loading

Be-ing commented Sep 28, 2021

Holzhaus commented Sep 28, 2021

JoergAtGithub commented Sep 28, 2021

uklotzde Sep 28, 2021

Choose a reason for hiding this comment

uklotzde Sep 28, 2021

Choose a reason for hiding this comment

uklotzde Sep 28, 2021

Choose a reason for hiding this comment

Be-ing Sep 28, 2021

Choose a reason for hiding this comment

uklotzde Sep 29, 2021

Choose a reason for hiding this comment

Be-ing Sep 30, 2021

Choose a reason for hiding this comment

ywwg commented Sep 30, 2021

Holzhaus commented Sep 30, 2021 • edited Loading

ywwg commented Sep 30, 2021

ywwg commented Sep 30, 2021

daschuer commented Oct 4, 2021

ywwg commented Oct 7, 2021

uklotzde commented Oct 7, 2021

ywwg commented Oct 7, 2021

ywwg commented Oct 7, 2021

uklotzde commented Oct 7, 2021

ywwg commented Oct 7, 2021

ywwg commented Oct 7, 2021

ywwg commented Oct 7, 2021 • edited Loading

ywwg commented Oct 7, 2021

daschuer commented Oct 7, 2021

daschuer commented Oct 7, 2021

Holzhaus commented Sep 28, 2021 •

edited

Loading

Holzhaus commented Sep 28, 2021 •

edited

Loading

Holzhaus commented Sep 30, 2021 •

edited

Loading

ywwg commented Oct 7, 2021 •

edited

Loading