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

Use mixxx::Bpm for some Beats functions #4044

Merged
merged 10 commits into from
Jul 6, 2021
Merged

Use mixxx::Bpm for some Beats functions #4044

merged 10 commits into from
Jul 6, 2021

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Jul 1, 2021

Follow up to #4043. This uses the proper mixxx::Bpm type in some method signatures related to beats. I didn't change all methods to keep the diff reasonably small.

@Be-ing Be-ing marked this pull request as draft July 1, 2021 21:58
@Be-ing
Copy link
Contributor

Be-ing commented Jul 1, 2021

Converted to draft until #4043 is merged.

@Holzhaus Holzhaus marked this pull request as ready for review July 2, 2021 16:54
@Holzhaus
Copy link
Member Author

Holzhaus commented Jul 2, 2021

This is ready now.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

In the future, we might also want to consider making (some parts of) the Bpm class constexpr.

src/library/dlgtrackinfo.cpp Show resolved Hide resolved
src/test/beatgridtest.cpp Show resolved Hide resolved
@Holzhaus
Copy link
Member Author

Holzhaus commented Jul 3, 2021

I added some simple tests for the class that we can extend later. A later PR improves the Bpm class further, so I'd rather improve the class there unless you consider it a merge blocker.

src/test/bpmtest.cpp Outdated Show resolved Hide resolved
EXPECT_EQ(mixxx::Bpm(120), mixxx::Bpm(240) / 2);

EXPECT_EQ(mixxx::Bpm(), mixxx::Bpm(mixxx::Bpm::kValueUndefined));
EXPECT_EQ(mixxx::Bpm(), mixxx::Bpm(0.0));
Copy link
Contributor

Choose a reason for hiding this comment

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

This last test is confusing. We could test that mixxx::Bpm::kValueUndefined equals 0.0 but that doesn't really make any sense to me. Otherwise we would need to test the value of each and every constant.

Copy link
Member Author

Choose a reason for hiding this comment

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

The point is that all invalid values should be equal to another. But it was actually not working, fixed that.

src/track/bpm.h Outdated
return bpm1.getValue() >= bpm2.getValue();
}

inline bool operator==(Bpm bpm1, Bpm bpm2) {
Copy link
Contributor

@uklotzde uklotzde Jul 3, 2021

Choose a reason for hiding this comment

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

I didn't notice this very special equality comparison! We should add a comment that all invalid values are considered equal.

We also should test the comparison of invalid and valid values.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will. But do you agree with this comparison? I think it makes sense to consider all invalid values equal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not saying we should imitate this...

❯ node
Welcome to Node.js v14.17.0.
Type ".help" for more information.
> NaN === NaN
false
>

https://www.destroyallsoftware.com/talks/wat

Copy link
Member Author

@Holzhaus Holzhaus Jul 3, 2021

Choose a reason for hiding this comment

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

This is actually the IEEE-754 standard. It's the same in C++. !(x == x) is a way to implement isNaN.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be surprising as the other comparison operators like <= behave differently! Do we really need it? Otherwise I would suggest to avoid these tweaks.

Copy link
Member Author

Choose a reason for hiding this comment

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

@uklotzde test added.

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be surprising as the other comparison operators like <= behave differently! Do we really need it? Otherwise I would suggest to avoid these tweaks.

I don't know. My rationale was that someValue == Bpm() is true if someValue is some invalid value.

Btw, I actually have a WIP PR that takes care of that and raises a debug assertion every time you try to do a calculation or comparison with an invalid value: 2217470#diff-6ee8fe6454f9e7e8637c1ba82b07d85ae618df4fbb651d66f8e5fff4b06f51e5

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be surprising as the other comparison operators like <= behave differently! Do we really need it? Otherwise I would suggest to avoid these tweaks.

I don't know. My rationale was that someValue == Bpm() is true if someValue is some invalid value.

I actually have a WIP PR that takes care of that and raises a debug assertion every time you try to do a calculation or comparison with an invalid value: 2217470#diff-6ee8fe6454f9e7e8637c1ba82b07d85ae618df4fbb651d66f8e5fff4b06f51e5

But I don't want to add all of that here. I can also revert these "all invalid values are equal" changes if you prefer.

@Holzhaus Holzhaus requested a review from uklotzde July 3, 2021 23:02
return (!bpm1.hasValue() && !bpm2.hasValue()) || bpm1.getValue() <= bpm2.getValue();
}

inline bool operator>(Bpm bpm1, Bpm bpm2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The definitions of the comparison operators are inconsistent, i.e. the result of lhs < rhs should equal !(lhs >= rhs).

Copy link
Member Author

Choose a reason for hiding this comment

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

In which cases does it not?

Copy link
Member Author

@Holzhaus Holzhaus Jul 5, 2021

Choose a reason for hiding this comment

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

Note that the < operator uses getValue() which throws a debug assert if at least one of the values is invalid.

50 < 100 = true
100 < 50 = false
100 < 100 = false
100 < invalid = debug assert
invalid < 100 = debug assert
invalid < invalid = debug assert

50 >= 100 = false
100 >= 100 = true
100 >= 100 = true
100 >= invalid = debug assert
invalid >= 100 = debug assert
invalid >= invalid = true (because they are equal)

Copy link
Contributor

@uklotzde uklotzde Jul 5, 2021

Choose a reason for hiding this comment

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

Independent of debug assertions these operators must be interchangeable. Everything else is unexpected.

The condition < must result in the same behavior as not >= because as a developer I expect that I could use either of them depending on which is more suitable for structuring the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I see an expression like if (!(a >= b)) I will refactor it to if (a < b).

Copy link
Member Author

Choose a reason for hiding this comment

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

getValue() always returns 0.0 if the value is invalid, so it should still work fine:

50 < 100 = true
100 < 50 = false
100 < 100 = false
100 < invalid = false + debug assert
invalid < 100 = true + debug assert
invalid < invalid = false debug assert

50 >= 100 = false
100 >= 100 = true
100 >= 100 = true
100 >= invalid = true + debug assert
invalid >= 100 = false + debug assert
invalid >= invalid = true

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually you only implement < and then the remaining operators in terms of this to prevent inconsistencies.

return bpm1.getValue() > bpm2.getValue();
}

inline bool operator>=(Bpm bpm1, Bpm bpm2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Both > and >= should delegate to < and <= with the arguments switched.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point.

Copy link
Member Author

Choose a reason for hiding this comment

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

@uklotzde
Copy link
Contributor

uklotzde commented Jul 5, 2021

Please fix the code style issues, then we can merge.

Copy link
Contributor

@uklotzde uklotzde 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! I guess the extra rounds during the review were worth the efforts ;) LGTM

@uklotzde uklotzde merged commit d4a6b2e into mixxxdj:main Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants