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

HLE Riivolution patch support #10127

Merged
merged 29 commits into from Oct 23, 2021
Merged

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Sep 26, 2021

Implemented in this PR right now:

  • All Riivolution features described on this page.
  • Ability to launch Riivolution-patched games in DolphinQt.
    • Right-click a game in the game list and select 'Start with Riivolution Patches...'.
    • By default XMLs and files will be loaded by assuming the directory one up from the XML as a virtual SD card root. This can be changed in the patch launcher window.
    • The default root directory is {user dir}/Load/Riivolution/.
    • Launching via the GUI like this remembers the last selected patches in the same way actual Riivolution does.
  • Ability to launch Riivolution-patched games in DolphinNoGUI.
    • Write a .json file like this and then point DolphinNoGUI at it.

Known desired features or issues that are (currently) not implemented in this PR:

  • A generic patch GUI, where Dolphin lists all patches installed for a game and allows you to select, prioritize, and organize them, which stay persistent across game sessions.
    • This will not be in this PR, and will likely not exist for a while. From what I know, @iwubcode wants to implement this in the future.
  • A way to launch Riivolution patches over Netplay or for TASing, and a way to have them show as extra entries in the game list for full game patches.
    • I'd like to do a follow-up PR for this that exposes the currently-CLI-only .json format in the GUI for this purpose.
  • Conflict resolution. If multiple patches target the same file, the last patch wins -- this is consistent with Riivolution itself, but not great.
    • Either we defer this to the future generic patch GUI or we have to think of something here. I'd be fine with just leaving it as-is for now.
  • The loading times are wrong due to the rebuilt FST.
    • I've worked around this by enabling the fast disc speed option for now when Riivolution patches are enabled, but if desired I can investigate making this actually accurate for this PR. Otherwise this will be a follow-up PR at some point in the future.

NOTE: Some of the specific details below may be a bit outdated, but the general gist still applies.

This implements the ability to parse and apply Riivolution XML files by rebuilding the game's FST before launch using DirectoryBlob.

A new subfolder in the user directory (Load/Riivolution) has been added to act as the virtual SD card root that Riivolution patches would normally search files in. Patch XMLs in Load/Riivolution/riivolution will also be parsed automatically when using this feature, mimicking how Riivolution loads XML files in (SD root)/riivolution.

To launch a game using this feature, right-click it in the game list and select Start with Riivolution... after placing the desired patch files in Load/Riivolution. Then select the patches you'd like to apply, and click Start.

So for example, to launch Newer Super Mario Bros. Wii you would do the following:

  • Download the mod from its official website.
  • Extract the NewerSMBW and riivolution folders into the Load/Riivolution directory. (example)
  • Launch Dolphin, right-click your vanilla New Super Mario Bros. game in the game list, and select Start with Riivolution....
  • For this patch the defaults are already correct, so just hit Start in the patch selection window. (example)
  • And enjoy!

step4

I'm open to suggestions for improving the user-experience, though I don't think the current way is terrible.

@AdmiralCurtiss AdmiralCurtiss force-pushed the riivolution branch 2 times, most recently from 48df0bc to 2b2888a Compare September 26, 2021 05:08
@sepalani
Copy link
Contributor

I'm fine with the current UI, it has a similar look and feel compared to Riivolution UI.

I tested this PR with UMH3+MH3SP-patcher Riivolution files. I also used AltWFC and MH3SP as server replacements.

Regarding MH3SP-patcher, some memory patches are working, the search ones using value do, the one using valuefile doesn't seem to work. I checked with the Memory widget and the value wasn't replaced.

It also seems the UMH3 title screen replacement isn't working. They look like this:

<file disc="/04/title_bg_eng.tpl" external="mh3t/textures/title/0.tpl" />

@AdmiralCurtiss
I haven't tried this PR on Newer yet. I can if you want me to.

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, 2 of 2 files at r5, 2 of 2 files at r6, 2 of 2 files at r7.
Reviewable status: 0 of 24 files reviewed, 2 unresolved discussions (waiting on @AdmiralCurtiss and @JosJuice)

a discussion (no related file):
I think the system of having to first select the game in the game list and then go to the Tools menu will be unintuitive for users. Currently the only actions in Dolphin that a user needs to do this for are certain TAS actions like starting recording a movie from boot, and I've seen it happen several times that TASers who are new to Dolphin don't understand why Start Recording is greyed out and what they're supposed to do in order to start recording. Having "Start with Riivolution" as a right-click option on games would be better.



Source/Core/DiscIO/DirectoryBlob.cpp, line 554 at r5 (raw file):

    const u64 data_size = partitions[i].partition.GetDataSize();
    const u64 partition_offset = partition_address + PARTITION_DATA_OFFSET;

I would suggest naming this partition_data_offset if it doesn't mess up the line widths too much. I try to always use "partition" when referring to the beginning of the whole partition and "partition data" when referring to the start of the encrypted+hashed area, whereas "address" and "offsets" are basically synonyms.

@iwubcode
Copy link
Contributor

So for example, to launch Newer Super Mario Bros. Wii you would do the following:

This is very cool @AdmiralCurtiss ! I can think of some usability improvements.

  1. Could these mods work like TexturePacks? If we could organize by gameid (or using the gameid.txt) we could have a more generic "enhancements" system in the future that works across the board for all our internal enhancements.
  2. Maybe add a "right click" to open up the mod folder?
  3. Instead of the "start with riivolution", could we just be able to right click on the game and select these in the same way we support "patches"? Again in the future, all these systems could be combined in a more intuitive interface. This would also mimic other emulators (ex: yuzu) and would support multiple mods at once.

I will give this a whirl soon. I have a lot of translations I want to check out.

@AdmiralCurtiss
Copy link
Contributor Author

Could these mods work like TexturePacks? If we could organize by gameid (or using the gameid.txt) we could have a more generic "enhancements" system in the future that works across the board for all our internal enhancements.

I did it like this so that the patches are as close as how you would install and use them on a real Wii -- which for most mods just means extracting the patch archive into the Riivolution folder (instead of the SD card root) and you're good. I'm not against 'future-proofing' this for a possible enhancement system, but I have no idea what the best way to accomplish that would be.

Maybe add a "right click" to open up the mod folder?

Where?

Instead of the "start with riivolution", could we just be able to right click on the game and select these in the same way we support "patches"? Again in the future, all these systems could be combined in a more intuitive interface. This would also mimic other emulators (ex: yuzu) and would support multiple mods at once.

Multiple mods at once are already supported here -- as they are on real Riivolution, for that matter! It will load all matching XMLs at once and you can just pick and choose what you want. I'm not the biggest fan of handling this like game cheats/patches because that would make it very easy to accidentally leave a mod on when you want to play the vanilla game, but ymmv.

@iwubcode
Copy link
Contributor

iwubcode commented Sep 26, 2021

Multiple mods at once are already supported here -- as they are on real Riivolution, for that matter! It will load all matching XMLs at once and you can just pick and choose what you want.

Any idea how it handles conflicts?

I'm not the biggest fan of handling this like game cheats/patches because that would make it very easy to accidentally leave a mod on when you want to play the vanilla game, but ymmv.

Fair. I was looking at this the opposite way. Thinking that if I most often want to play with the mod, I have to remember to use this sub-menu to launch it. I can't just double click on it. Plus, it's very possible I want to trigger it from the command line (ex: frontend launchers). Having a separate option for that might be a bit klunky (just my opinion of course).

I'm not against 'future-proofing' this for a possible enhancement system, but I have no idea what the best way to accomplish that would be.

If we could just move these files one level down, ex add a gameid layer, that'd be nice. Just to make it clearer what mods belong to what game.

Where?

I was thinking right click on the game itself. If we had a gameid layer, we could open up the gameid folder itself (and if it doesn't exist create it before opening). This would make it easy to see what mods were associated with each game.

Even if we didn't have a gameid layer, I think it'd be intuitive to add it to the game context menu.


Riivolution is for Wii retail only right? just curious, is there anything similar for GC/Wiiware? Wondering if Load/Mods would be more future proof?

@JosJuice
Copy link
Member

If we could just move these files one level down, ex add a gameid layer, that'd be nice. Just to make it clearer what mods belong to what game.

Riivolution XML files already identify what game ID they're for. (See the IsValidForGame function in this PR.)

I think the main result of requiring a game ID folder structure would be to make this feature harder for users to set up. Getting them to name their texture pack folders correctly is already hard as it is... (Or at least it was before the texture pack folder naming system was improved. I'm don't think I actually see that many problems with it nowadays.)

@iwubcode
Copy link
Contributor

Thanks @JosJuice . If we have a way to find all the mods for a given game (ex: ability to list all enhancements in the UI for a given gameid), I think we're at least future-proofed in that sense. Updated my comments.

@iwubcode
Copy link
Contributor

iwubcode commented Sep 26, 2021

One other comment, if we do keep the current interface, please switch to having "Start" as the default button. That way after clicking to begin the game, I can just hit enter to play.

That being said, I tried a handful of mods. They all worked very well!!

@JMC47
Copy link
Contributor

JMC47 commented Sep 26, 2021

Was pretty painless to setup for the most part. I think the biggest issue is figuring out to highlight the game if you're not used to recording TASes.

@Zopolis4
Copy link
Contributor

Does this have a priority order like real riivolution? Also, is the intended method to copy the riivolution folders for each respective mod into the main riivolution folder in Load/Riivolution/riivolution?

@AdmiralCurtiss
Copy link
Contributor Author

AdmiralCurtiss commented Sep 27, 2021

Does Riivolution have a priority system? I haven't seen anything along those lines but maybe I've just overlooked it. Right now I'm not doing any conflict handling or similar, and I was under the impression Riivolution didn't either.

As for the other question, just treat it like you would on a real Wii with an SD card. All *.xml files go into Load/Riivolution/riivolution, the actual mod files go wherever the mod wants them to be under the assumption that Load/Riivolution is the SD card root.

@Zopolis4
Copy link
Contributor

Riivolution handles conflicts via the vertical order of the mods, with the higher mods overriding the other mods, iirc.

@AdmiralCurtiss
Copy link
Contributor Author

And how is that order decided (especially across multiple XML files)...?

@Zopolis4
Copy link
Contributor

Dragging and dropping with wiimote, I think. Not sure about the backend, but the source code probably shows.

@AdmiralCurtiss
Copy link
Contributor Author

Oh, I didn't realize that was possible! Okay, thanks, I'll research the exact details.

@AdmiralCurtiss AdmiralCurtiss force-pushed the riivolution branch 3 times, most recently from aef348c to 224d770 Compare September 27, 2021 02:55
@AdmiralCurtiss
Copy link
Contributor Author

AdmiralCurtiss commented Sep 27, 2021

@sepalani Can you try the Monster Hunter patches again? The XML files didn't set any root patch, which I assumed meant 'relative to SD root' but seems to instead mean 'relative to folder containing xml'. They also used Ocarina patches, which were unimplemented before -- I've implemented them now, I think.

@sepalani
Copy link
Contributor

@AdmiralCurtiss
That seems to work now:

RMHE08_2021-09-27_13-07-49
RMHE08_2021-09-27_13-14-52

@AdmiralCurtiss
Copy link
Contributor Author

@Zopolis4 Are you absolutely sure Riivolution on console has a priority feature? I just spent some time messing with it and was unable to find any such thing. Additionally, if I enable multiple patches for the same file, they just seem to get applied in the order they're listed in the XML file, later patches for any given file or file region overwriting whatever happens to be there at the time -- which is identical to what I'm doing here.

One thing I ran into that I didn't realize before is that apparently the offset in mid-file patches is limited to 4 byte alignments, getting aligned down (that is, address & ~3) when it's in-between. I should add that.

Also, I'm still not fully treating external paths accurately. If they start with a '/' they seemingly are relative to the SD card root, regardless of what is given as the 'root' parameter. Which makes some sense, I guess.

Fixes for both of these will follow shortly...

@Zopolis4
Copy link
Contributor

I may be wrong, I'm mainly going off of memory here. I trust your judgment on this, especially as I could not find any relevant mentions of priority in riivolution's source code.

@iwubcode
Copy link
Contributor

later patches for any given file or file region overwriting whatever happens to be there at the time -- which is identical to what I'm doing here.

I need to look through the code but how do you signify a later patch?? These are just files in the directory, are they loaded in name order?

@AdmiralCurtiss
Copy link
Contributor Author

I mean, like, if you did this:

<wiidisc version="1" root="_test">
  <id game="SUKE" />
  <options>
    <section name="Sec1">
      <option id="A" name="A" default="1">
        <choice name="a">
          <patch id="a" />
        </choice>
      </option>
      <option id="B" name="B" default="0">
        <choice name="b">
          <patch id="b" />
        </choice>
      </option>
      <option id="C" name="C" default="0">
        <choice name="c">
          <patch id="c" />
        </choice>
      </option>
    </section>
  </options>
  <patch id="a">
    <file disc="/msg/US_English/TitleMenu.msbt" external="_A" />
  </patch>
  <patch id="b">
    <file disc="/msg/US_English/TitleMenu.msbt" external="_B" />
  </patch>
  <patch id="c">
    <file disc="/msg/US_English/TitleMenu.msbt" external="_C" />
  </patch>
</wiidisc>

If you enable all three of these patches, file _C ends up being used, presumably because it patches them over the data in order. Similarly, if you assume those replacement files are all, say, 8 bytes long and did something like

  <patch id="a">
    <file disc="/msg/US_English/TitleMenu.msbt" external="_small1" offset="0x71c" resize="false" />
  </patch>
  <patch id="b">
    <file disc="/msg/US_English/TitleMenu.msbt" external="_small2" offset="0x724" resize="false" />
  </patch>
  <patch id="c">
    <file disc="/msg/US_English/TitleMenu.msbt" external="_small3" offset="0x720" resize="false" />
  </patch>

Then you end up with the first 4 bytes of _small1, followed by the full contents of _small3, followed by the last 4 bytes of _small2 -- presumably because it overlays the three patches in that order.

With folder patches and recursive file replacement this gets more convoluted to test, but I'd presume it's similar?

@AdmiralCurtiss
Copy link
Contributor Author

AdmiralCurtiss commented Sep 29, 2021

If you mean the order the XMLs are being loaded if you have multiple and how that would affect the results, very good question. I'm not sure anyone ever fully considered this logic.

e: Checking the actual Riivolution source,
https://github.com/AerialX/rawksd/blob/14b6f27cfb3ab1e4a2288a34f145f844ca96bd4a/launcher/source/riivolution_config.cpp#L571-L599
https://github.com/AerialX/rawksd/blob/14b6f27cfb3ab1e4a2288a34f145f844ca96bd4a/filemodule/libfile/files.c#L201-L224

Looks like it just reads them as they get returned by the filesystem (IOCTL_NextDir, where exactly is SD card reading implemented in Dolphin...?), so technically completely arbitrary? Depends on what the IOS FAT code does, probably...?

e2: wait, the FAT code is implemented in riivolution:
https://github.com/AerialX/rawksd/blob/14b6f27cfb3ab1e4a2288a34f145f844ca96bd4a/filemodule/source/file_fat.cpp#L197-L206
https://github.com/AerialX/rawksd/blob/14b6f27cfb3ab1e4a2288a34f145f844ca96bd4a/filemodule/libfat/source/wrapper.c#L42-L48
https://github.com/AerialX/rawksd/blob/14b6f27cfb3ab1e4a2288a34f145f844ca96bd4a/filemodule/libfat/source/fat/fatdir.c#L576-L608
https://github.com/AerialX/rawksd/blob/14b6f27cfb3ab1e4a2288a34f145f844ca96bd4a/filemodule/libfat/source/fat/directory.c#L289-L394

Which still looks to me that it's just whatever order the files happen to be stored in the FAT on the SD card, so completely arbitrary. Unfortunate!

@iwubcode
Copy link
Contributor

iwubcode commented Sep 29, 2021

Which still looks to me that it's just whatever order the files happen to be stored in the FAT on the SD card, so completely arbitrary. Unfortunate!

Does that mean filename ordering?

--

I question if we should be HLE'ing the actual riivolution loading as opposed to us owning the patch ordering and riivolution just being one type of patch. Priority handling is important and sounds like an oversight from the Riivolution team. Also, as mentioned before, I feel like the folder inside 'Load' should be called mods so it is generic. This allows us to support other patch formats in the future (ex: the Pokemon Mystery Dungeon translation patches are bps because riivolution only applies to Wii retail discs).

@AdmiralCurtiss
Copy link
Contributor Author

AdmiralCurtiss commented Sep 29, 2021

Does that mean filename ordering?

Depends on the code that writes the FAT in the first place I guess? I think in practice you can make no assumptions, though of course we could define a logic for it for consistency (even if that may be inconsistent with actual Riivolution).


I do agree that supporting other types of patches makes sense, but the specifics of how you'd actually like that to be implemented are rather nebulous to me. I'm also not sure if it really makes sense to mix together multiple types of patches in the same place in the file system. Maybe make mockup or something so I have a better idea what you want here?

…uring the patching process to properly resolve the paths given in the XML.

This also may eventually allow loading patches from sources other than the 1:1 expected file structure host file system, such as memory or an archive file.
…hed games until we have proper emulation for that.
…h options when launching via the RiivolutionBootWidget.
…peration may be on different partitions or devices. Implement a fallback for that.
@IonicPixels
Copy link

I've used this for a while and its great, but I do suggest making a change so that the selected Riivo Patches will stay enabled after loading the game, like in Riivolution. (Riivolution has a config folder that it keeps the enabled mods stored)

@AdmiralCurtiss
Copy link
Contributor Author

Uh, that's in there as of a few commits ago. Or do you mean that the patches should stay 'on' even when you boot the game without the Riivolution UI? That's not going to happen like that (in this PR) because we have a different thing planned.

@IonicPixels
Copy link

Uh, that's in there as of a few commits ago. Or do you mean that the patches should stay 'on' even when you boot the game without the Riivolution UI? That's not going to happen like that (in this PR) because we have a different thing planned.

yeah I meant patches should stay on lol

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r23, 3 of 3 files at r24, all commit messages.
Dismissed @sepalani from 4 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AdmiralCurtiss)

@leoetlino leoetlino merged commit 5d5f019 into dolphin-emu:master Oct 23, 2021
9 of 10 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the riivolution branch October 23, 2021 22:58
@IonicPixels

This comment has been minimized.

@AdmiralCurtiss
Copy link
Contributor Author

That's... just how you do things on Github, and git in general. Make a branch, work on it, merge into master when it's done, then delete the branch as it's no longer useful anymore -- all the data is now in master. Keeps your repository tidy.

@IonicPixels
Copy link

I have never actually seen a branch get deleted after a merge...

@RoadrunnerWMC
Copy link

AdmiralCurtiss is right -- GitHub even nudges you to do it after you merge a PR. (The message is something like "You're all set -- the original branch can now be safely deleted. ['Delete Branch' button]")

@IonicPixels
Copy link

AdmiralCurtiss is right -- GitHub even nudges you to do it after you merge a PR. (The message is something like "You're all set -- the original branch can now be safely deleted. ['Delete Branch' button]")

oh, I never knew that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet