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

Add MMR custom music support to OOTR. .OOTRS file improvements #2044

Merged
merged 10 commits into from
Sep 30, 2024

Conversation

rrealmuto
Copy link

This PR adds the ability to import MMR's custom music directly into OOTR.

Also updates how custom instruments get added into the .ootrs meta files while remaining backwards compatible with the current format.

As far as I know this is working but I'm not as good at testing this stuff as @owl_is_not_a_cat

@rrealmuto
Copy link
Author

I'll squash before marking this as ready 😅

@fenhl fenhl added Type: Enhancement New feature or request Component: Cosmetics Affects the patching of cosmetics labels Jul 15, 2023
@cjohnson57 cjohnson57 added Status: Waiting for Release This PR is ready for merge, but we're holding off on it until after the next release Status: Needs Review Someone should be looking at it Status: Needs Testing Probably should be tested and removed Status: Waiting for Release This PR is ready for merge, but we're holding off on it until after the next release labels Nov 11, 2023
@rrealmuto rrealmuto mentioned this pull request Nov 17, 2023
4 tasks
@rrealmuto
Copy link
Author

rrealmuto commented Nov 21, 2023

So I'm thinking that this is ready to merge after resolving CI checks + conflicts. A handful of folks have used it on my branch without issues. There are a few things that may be broken but I wouldn't consider them a show-stopper for merging this but wanted to get your thoughts:

  • MMR supports what is known as a "mask form" sequence. Basically it's a single sequence that changes based on whatever mask the player is wearing. We don't really have a way to properly support this in OOTR. I'm not exactly sure how they will sound in OOTR but I think it might play all 3 simultaneously. If anyone knows of a good way to detect this, perhaps we could just do that and ignore the sequence.
  • MMR has some .mmrs files with multiple banks. Apparently this carried over from when they had very early support for custom instruments. I don't believe it breaks on our end though I haven't tested it all that much.
  • There are a handful of MMR .zseq files that don't conform to any reasonable naming standard. We just don't support those :)

@rrealmuto rrealmuto marked this pull request as ready for review February 10, 2024 05:48
@fenhl fenhl removed the Status: Needs Testing Probably should be tested label Mar 21, 2024
@rrealmuto
Copy link
Author

With the last commit this should remain compatible with the new web patcher. Obviously the web patcher won't support the MMR music, but this shouldn't break Dev once it's merged.

@fenhl
Copy link
Collaborator

fenhl commented Sep 17, 2024

@owlisnotacat1 would you be willing to give this a code review?

Copy link

@owlisnotacat1 owlisnotacat1 left a comment

Choose a reason for hiding this comment

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

Looks good, I couldn't find any errors and I've been regularly playing on this branch. 👍

@owlisnotacat1
Copy link

@owlisnotacat1 would you be willing to give this a code review?

Sorry about the late response I've been swamped with stuff recently

@fenhl
Copy link
Collaborator

fenhl commented Sep 25, 2024

No worries, thank you for the review!

@fenhl fenhl added Status: Waiting for Release This PR is ready for merge, but we're holding off on it until after the next release and removed Status: Needs Review Someone should be looking at it labels Sep 25, 2024
@fenhl fenhl removed the Status: Waiting for Release This PR is ready for merge, but we're holding off on it until after the next release label Sep 30, 2024
@fenhl fenhl added this to the next milestone Sep 30, 2024
@fenhl fenhl merged commit ba82e6b into OoTRandomizer:Dev Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Cosmetics Affects the patching of cosmetics Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants