Skip to content

Rework the MT-32/CM-32L ROM lookup and MIXER /LISTMIDI improvements #3011

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

Merged
merged 7 commits into from
Oct 23, 2023

Conversation

johnnovak
Copy link
Member

@johnnovak johnnovak commented Oct 16, 2023

Description

Now that we have moved from filename-based MT-32/CM-32L ROM lookup to hash-based, the unversioned vs versioned ROM differentiation doesn't make much sense.

mt32_old and mt32_new linking to specific v1.x and v2.x MT-32 versions, respectively, was also a pretty arbitrary decision and was in conflict with how the auto model worked.

This change makes the lookup behaviour of "symbolic" models uniform and consistent (auto, mt32, cm32l, mt32_old, and mt32_new). The lookup mechanism of models that specify the version number (e.g., mt32_107, cm32l_102) is unchanged.

The output of MIXER /LISTMIDI is also improved.

Previous behaviour

  • Unversioned ROMs were expected to exist with the MT32_CONTROL.ROM & MT32_PCM.ROM filenames for MT-32, and MT-32 CM32L_CONTROL.ROM & CM32L_PCM.ROM for the CM-32L.
  • Versioned ROMs were expected to be named according to the MAME MT-32/CM-32L file naming convention.
  • model = auto loaded the first existing ROM in priority order as specified in the config description of model.
  • model = mt32 attempted to load the unversioned MT-32 ROM and fail the ROM file did not exist.
  • model = cm32l attempted to load the unversioned CM-32L ROM and fail the ROM file did not exist.
  • model = mt32_new attempted to load the versioned v2.04 MT-32 ROM and fail if that exact ROM file did not exist.
  • model = mt32_old attempted to load the versioned v1.07 MT-32 ROM and fail if that exact ROM file did not exist.

New revised behaviour

  • We drop the concept of versioned and unversioned ROMs completely. Valid MT-32/CM-32L ROMs are identified by their checksums (hashes) at startup, the filenames do not matter. Every identified ROM file is tied to a concrete version number, naturally—it can't work any other way.
  • model = auto loads the first existing ROM in priority order as specified in the config description of model. This is the same behaviour as before.
  • model = mt32 loads the first MT-32 ROM using the same priority order (it fails if only CM-32L ROMs are present or no ROMs at all).
  • model = cm32l loads the first CM-32L ROM in priority order (it fails if only MT-32 ROMs are present or no ROMs at all).
  • model = mt32_new loads the first v2.x MT-32 ROM in priority order (it fails only if no v2.x MT-32 ROMs are present).
  • model = mt32_old loads the first v1.x MT-32 ROM in priority order (it fails only if no v1.x MT-32 ROMs are present).

New model setting description

Basically, a succinct version of the above:

image

Backward compatibility

In some weird edge cases (point 3), the behaviour can be different than before (meaning a different ROM model might be picked up while the contents of the romdir is exactly the same and the user's config is exactly the same too). However, I think this is a complete non-issue as most people would fall into the sane 1) and 2) use cases. People doing weird shit as described in 3) will need to fix up their configs 😎

  1. romdir has the "unversioned" ROM files only. In this legacy use case, model = mt32 will pick the MT-32 ROM and model = cm32l the CM-32L ROM. Easy!

  2. romdir has all versioned ROM files (recommended setup). In this case, everything works as expected, even a. bit better (e.g., for mt32_new we'll still pick the "best" available "new" MT-32 ROM, even if v2.04 is missing).

  3. Mixed case when romdir contains both versioned and unversioned ROM files, and the person uses mt32 or cm32l which were previously referring to the "unversioned" files—well, depending on what version the "unversioned" ROMs are, and what "versioned" ROMs the person has, the behaviour can be... pretty much anything 😄 However, this use case was always a bit of a clusterfuck and normally people who still use "legacy unversioned ROMs" fall into the first category. The behaviour with mixed versioned and unversioned ROMs was pretty confusing previously and it wasn't a good idea to do that anyway.

Related issues

Fixes:

Manual testing

Tested for regressions when using exact model versions and the symbolic models. The symbolic model resolution is more interesting, so here's the test evidence for that:

image

image

image

Hercules output

image

Checklist

Please tick the items as you have addressed them. Don't remove items; leave the ones that are not applicable unchecked.

I have:

  • followed the project's contributing guidelines and code of conduct.
  • performed a self-review of my code.
  • commented on the particularly hard-to-understand areas of my code.
  • split my work into well-defined, bisectable commits, and I named my commits well.
  • applied the appropriate labels (bug, enhancement, refactoring, documentation, etc.)
  • checked that all my commits can be built.
  • confirmed that my code does not cause performance regressions (e.g., by running the Quake benchmark).
  • added unit tests where applicable to prove the correctness of my code and to avoid future regressions.
  • made corresponding changes to the documentation or the website according to the documentation guidelines.
  • locally verified my website or documentation changes.

@johnnovak johnnovak force-pushed the jn/midi-improvements-1 branch 3 times, most recently from c0f7679 to ff54768 Compare October 19, 2023 06:48
@johnnovak johnnovak added midi MIDI related features and issues enhancement New feature or enhancement of existing features regression We broke something 😊 labels Oct 19, 2023
@johnnovak johnnovak self-assigned this Oct 19, 2023
@johnnovak johnnovak force-pushed the jn/midi-improvements-1 branch from ff54768 to 6bf55ff Compare October 19, 2023 08:27
@johnnovak johnnovak changed the title DRAFT Jn/midi improvements 1 Rework/improve the MT-32/CM-32L ROM lookup and MIXER /LISTMIDI improvements Oct 19, 2023
@johnnovak johnnovak changed the title Rework/improve the MT-32/CM-32L ROM lookup and MIXER /LISTMIDI improvements Rework the MT-32/CM-32L ROM lookup and MIXER /LISTMIDI improvements Oct 19, 2023
@johnnovak johnnovak marked this pull request as ready for review October 19, 2023 08:49
@johnnovak
Copy link
Member Author

TODO: We'll need to update the MIDI section of the wiki that details the MT-32/CM-32L ROM lookup mechanism.

Copy link
Member

@kcgen kcgen left a comment

Choose a reason for hiding this comment

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

That's a big improvement, @johnnovak - very nice!
Just a couple suggestions.

@johnnovak johnnovak force-pushed the jn/midi-improvements-1 branch from 6bf55ff to 322b086 Compare October 22, 2023 01:10
@johnnovak
Copy link
Member Author

johnnovak commented Oct 22, 2023

Had a bit of time @kcgen so I've addressed your comments & have rebased to latest main. Thanks for the thorough review ❤️

@johnnovak johnnovak force-pushed the jn/midi-improvements-1 branch from 322b086 to 3c9a7dd Compare October 22, 2023 01:12
@kcgen kcgen self-requested a review October 22, 2023 05:52
@johnnovak johnnovak force-pushed the jn/midi-improvements-1 branch from 3c9a7dd to 2046e60 Compare October 22, 2023 08:42
No functional changes.
Treating CM-32LN as the "best" CM-32* model is the wrong choice as the
LN models feature a faster vibrato than the older L models. Most game
music was composed for the older L models, which makes some soundtracks
sound a bit weird on the newer LN models.
Now that we have moved from filename-based MT-32/CM-32L ROM lookup to
hash-based, the unversioned vs versioned ROM differentiation doesn't
make much sense.

`mt32_old` and `mt32_new` linking to specific v1.x and v2.x MT-32 versions,
respectively, was also a pretty arbitrary decision and was in conflict
with how the auto model worked.

This change makes the lookup behaviour of "symbolic" models uniform and
consistent (`auto`, `mt32`, `cm32l`, `mt32_old`, and `mt32_new`). The
lookup mechanism of models that specify the version number (e.g.,
`mt32_107`, `cm32l_102`) is unchanged.

The output of `MIXER /LISTMIDI` is also improved.
@FeralChild64
Copy link
Collaborator

No remarks from my side. When testing the command I have discovered an issue #3046, but this is not a regression, at least comparing to 0.80.1.

@johnnovak johnnovak merged commit 058dd72 into main Oct 23, 2023
@johnnovak johnnovak deleted the jn/midi-improvements-1 branch October 24, 2023 23:12
@johnnovak johnnovak removed the regression We broke something 😊 label Nov 13, 2023
@johnnovak johnnovak added the audio Audio related issues or enhancements label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audio Audio related issues or enhancements enhancement New feature or enhancement of existing features midi MIDI related features and issues
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants