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

Support newer OMR format #1682

Merged
merged 5 commits into from
Sep 3, 2023
Merged

Support newer OMR format #1682

merged 5 commits into from
Sep 3, 2023

Conversation

fsvgm777
Copy link
Contributor

@fsvgm777 fsvgm777 commented Sep 2, 2023

Fixes #1586.

Also fixes a rounding issue when calculating the total frame count, as it'd result in the total run time being wrong most of the time.

Included are also additional system detection tests to account for the recent format changes.

I've tested it locally, and it works fine.

@fsvgm777
Copy link
Contributor Author

fsvgm777 commented Sep 3, 2023

After a brief discussion on Discord, I am not sure whether the rounding should be changed back (Math.Ceiling) or if the rounding should be kept (with the midpoint rounding changed to AwayFromZero to fix more rounding issues). As the format doesn't exactly have a concept of a frame per se (in fact, from my understanding of the format, it's cycle-based, which can then be converted to seconds), I wouldn't even be opposed to put in a frame rate override. Opinions?

My main thing with changing it back to Math.Ceiling is that it would then result in the wrong run time being displayed on the site (e.g. Skooter would have a run time of 5:41.44 instead of 5:41.42, and I'm more convinced the latter is right, based on how the time is calculated (you end up with 341.42295459339107 seconds after dividing the last key matrix state in that particular movie (1173253276800) by 3579545.0, and then by 960.0).

@MBilderbeek
Copy link

I'd approve this if I had the power 🙂

@adelikat adelikat merged commit 5f58cbb into TASVideos:main Sep 3, 2023
1 check passed
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.

System detection for openMSX movie files (.omr) post-18.0 is broken
3 participants