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

FramePos improvements #4053

Merged
merged 4 commits into from
Jul 4, 2021
Merged

FramePos improvements #4053

merged 4 commits into from
Jul 4, 2021

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Jul 3, 2021

Some minor improvements to the FramePos class added in #4043.

@Holzhaus
Copy link
Member Author

Holzhaus commented Jul 3, 2021

This should ensure that all calculations with frame positions are well defined, and we never end up with weird Nan values in the engine.

@Holzhaus Holzhaus requested a review from uklotzde July 4, 2021 15:19
@uklotzde
Copy link
Contributor

uklotzde commented Jul 4, 2021

I am still worried about the inconsistencies introduced by the special equality comparisons ==/!= that are incompatible with </<=/>/>=.

How about reverting them and only adding the assertion DEBUG_ASSERT(lhs.isValid() == rhs.isValid())? No, that doesn't make any sense ;)

@Holzhaus
Copy link
Member Author

Holzhaus commented Jul 4, 2021

Actually the striked through suggestion from your comment is already the case.

The comparison operators all use value() which raises a debug assertion if the value is invalid. Exceptions are == and !=. There return true if both positions are valid and their value matches, or both are invalid. Otherwise they return false.

@Holzhaus
Copy link
Member Author

Holzhaus commented Jul 4, 2021

What kind of issues do you expect?

IMHO allowing comparisons of valid and invalid values is more confusing. If you
make a check loopStart <= loopEnd and due to some engine bug, loopEnd is set to infinity (and the FramePos is invalid), the check would return true and you won't notice. With this implementation, a debug assertion is raised, which indicates your assumption that both values are valid is wrong.

Do you have a use case for comparisons between valid and invalid values, or two invalid values?

@uklotzde
Copy link
Contributor

uklotzde commented Jul 4, 2021

I didn't consider the debug assertions that already prevent using </<=/>/>= with invalid values. Then there should be no inconsistencies.

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! LGTM

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

Successfully merging this pull request may close these issues.

2 participants