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

Remove NKit data when extracting a GCN/Wii disc volume #11863

Merged
merged 1 commit into from Jun 12, 2023

Conversation

Minty-Meeo
Copy link
Contributor

@Minty-Meeo Minty-Meeo commented May 30, 2023

NKit stores some data in the disc and partition headers. Since the extracted filesystem format (GCReEx format?) cannot be an NKit format, it makes sense to remove this info. This will be helpful for a niche scenario in which a user extracts an NKit format ISO using Dolphin, then rebuilds the ISO using either Dolphin (which surprisingly is able to) or a third party tool, as the NKit data being left in will produce false-positives when loading the rebuilt disc in Dolphin Emulator.

@Minty-Meeo Minty-Meeo marked this pull request as draft May 30, 2023 08:16
@Minty-Meeo Minty-Meeo force-pushed the nkit-extract branch 2 times, most recently from c3e2a29 to 5b57289 Compare May 30, 2023 08:43
@Minty-Meeo Minty-Meeo marked this pull request as ready for review May 30, 2023 10:26
@JosJuice
Copy link
Member

I would prefer to not keep having to add special handling for NKit files throughout the Dolphin code base. Is this really something that happens often to people who don't have the technical ability to fix the problem after the fact?

For what it's worth, extracted disc images do in fact have almost all the problems that NKit files have.

@Minty-Meeo
Copy link
Contributor Author

If you don't see the value in handling a popular format correctly, unofficial it may be, then I don't know how to convince you.

@Extrems
Copy link

Extrems commented May 31, 2023

There's already a game mod out there with this issue. I think Dolphin should prevent this from happening again.

@Minty-Meeo
Copy link
Contributor Author

Yes, so as I discussed and learned from the NKit discord, then mentioned in the IRC, I encountered a Pikmin 2 mod which made this exact mistake. While not all tools can be perfect, at least Dolphin can help prevent this user error.

@JosJuice
Copy link
Member

Okay, I won't block this PR then. But I would like someone else to merge it.

@Minty-Meeo
Copy link
Contributor Author

With that resolved... I had considered making the DiscIO::ExportData function return the File::IOFile class it constructs, allowing the caller to either immediately convert it to a bool, or carry on using it. This would remove the need to open the file a second time in ExportWiiUnencryptedHeader and ExportHeader for these minor edits, though due to the File::IOFile operator bool overload being explicit, I thought it looked kind of ugly. Would this still be preferred over opening the file a second time?

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.

Sorry, I wrote three review comments yesterday but then somehow forgot to actually send them. Here they are.

Source/Core/DiscIO/DiscExtractor.cpp Outdated Show resolved Hide resolved
Source/Core/DiscIO/DiscExtractor.cpp Outdated Show resolved Hide resolved
Source/Core/DiscIO/DiscExtractor.cpp Outdated Show resolved Hide resolved
@JMC47
Copy link
Contributor

JMC47 commented Jun 2, 2023

Should we note the game mod that actually bugs out on this? Or are there multiple? I'm also unsure what to do about this because the format is rather weird, but considering so many people currently use it, I'm not sure we have a huge choice and obscure bugs are the worst.

@Minty-Meeo
Copy link
Contributor Author

I forgot to reply to this, my apologies. This is the mod's release video. While it is the only example I know of, it has become popular in the Pikmin 2 hacking community to teach new modders to use Dolphin Emulator for extracting, editing, and rebuilding a game. I believe it is inevitable for new modders to repeat this mistake, perhaps spreading to other Gamecube modding communities that similarly have no qualms publishing mods in such a way...
From my brief reading of the NKitv1 source code, I believe Dolphin Emulator's current NKit detection is about as good as it can get. Any other info found in the NKit data I think is only verifiable after converting back to an ISO.

@JMC47
Copy link
Contributor

JMC47 commented Jun 6, 2023

I'm not exactly happy about adding special handling here, but considering how important modding is and how prevalent nkit is among users, I don't see a good way around this. If nkit falls out of favor in the future, we can remove this special handling pretty easily.

I'll give others some time to object, but otherwise I'll merge this in a day or two. Feel free to ping me if I forget.

@Testsr
Copy link

Testsr commented Jun 12, 2023

@JMC47 Today is day n of waiting for you to merge.

@JMC47
Copy link
Contributor

JMC47 commented Jun 12, 2023

No one has objected and it's been a week. I guess we do it.

@JMC47 JMC47 merged commit c8559a7 into dolphin-emu:master Jun 12, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants