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

DolphinTool: Allow converting non-GC/Wii files #10507

Merged

Conversation

JosJuice
Copy link
Member

This was requested by a forum user, and I thought why not. It's a simple change to make since DiscIO already supports it, and I assume command-line users know roughly what they're doing.

return 1;
}

// Open the volume
std::unique_ptr<DiscIO::Volume> volume = DiscIO::CreateDisc(input_file_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we expose the std::unique_ptr<VolumeDisc> TryCreateDisc(std::unique_ptr<BlobReader>& reader) function and use it here so we don't create redundant blob readers?

Copy link
Member Author

Choose a reason for hiding this comment

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

That wouldn't work. TryCreateDisc moves the blob reader if the operation is successful, and we need to be able to keep using the blob reader after creating the volume.

Do you think adding a GetBlobReader function to the Volume class could make sense instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, fair enough.

The BlobReader appears to be stateless (ie, we don't require its user to remember the last read position or anything) so we can probably do that, yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, but if I move the blob reader into CreateDisc(std::unique_ptr<BlobReader>), the blob reader is gone if creating the volume fails... And we can't know if creating the volume will fail until we actually call it. So we would still need to expose TryCreateDisc, which I'm a little reluctant to do because its behavior of sometimes moving what you pass in is non-intuitive.

This problem is a bit tricker than I thought. Could we skip it for this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sure, it's not really a big deal, I just thought about it while reading the diff.

This was requested by a forum user, and I thought why not.
It's a simple change to make since DiscIO already supports it,
and I assume command-line users know roughly what they're doing.
success = DiscIO::ConvertToGCZ(blob_reader.get(), input_file_path, output_file_path,
volume->GetVolumeType() == DiscIO::Platform::WiiDisc ? 1 : 0,
{
u32 sub_type = std::numeric_limits<u32>::max();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an existing convention for non-GC/non-Wii GCZ files to have a sub_type of 0xFFFFFFFF, or did you make this up?

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it up. Dolphin has never had the ability to convert non-GC/Wii files to GCZ before, and there's no documentation on the format other than the source code.

@AdmiralCurtiss
Copy link
Contributor

Untested, but looks good to me.

@AdmiralCurtiss
Copy link
Contributor

Converted a few isos back-and-forth, worked fine.

@AdmiralCurtiss AdmiralCurtiss merged commit 1a5a52c into dolphin-emu:master Mar 23, 2022
10 checks passed
@JosJuice JosJuice deleted the convertcommand-non-gc-wii branch March 23, 2022 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants