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 a Verify tab to game properties #7922

Merged
merged 10 commits into from Apr 11, 2019
Merged

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Mar 23, 2019

This PR adds a new tab to the game properties window where you can check for problems with your games. Hopefully this can cut down a bit on people who ask about problems that turn out to be caused by bad dumps. For instance, there's a message about not having combined the files that CleanRip produces, and there's a message you get if you've pirated a single-layer version of a game that's supposed to be dual-layer. Some screenshots to show what it looks like:

My good dump of Rodea the Sky Soldier (NTSC-K):

image

The same game but with the IOS slot hex edited to reproduce https://bugs.dolphin-emu.org/issues/10319:

image

This is a WIP for now, but feel free to review, as long as it isn't about the Process() function (I'm going to change it later). Left to do:

  • Integrate checksum calculation (currently in the Info tab)
  • Integrate the "Check Partition Integrity" feature (currently in the Filesystem tab)
  • Show a different error message when Wii hash errors are in unused blocks
  • Check hashes of WAD contents
  • Dragon Quest X?

It would also be nice to include the ability to verify hashes with Redump.org or a similar database (https://bugs.dolphin-emu.org/issues/10867), but that's complex enough that I don't want to do it in this PR.

@JosJuice
Copy link
Member Author

Seems like lint doesn't understand that a CJK character isn't three columns wide just because it takes up 3 bytes in UTF-8. Oh well.

Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

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

Like the idea 👍

Source/Core/DiscIO/VolumeVerifier.cpp Outdated Show resolved Hide resolved
Source/Core/DiscIO/VolumeVerifier.cpp Outdated Show resolved Hide resolved
Source/Core/DiscIO/VolumeVerifier.cpp Outdated Show resolved Hide resolved
Source/Core/DiscIO/VolumeVerifier.cpp Outdated Show resolved Hide resolved
Source/Core/DiscIO/VolumeVerifier.cpp Show resolved Hide resolved
Source/Core/DiscIO/VolumeVerifier.h Show resolved Hide resolved
const DiscIO::VolumeVerifier::Problem problem = result.problems[i];

QString severity;
switch (problem.severity)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to use shading or text color to indicate severity as well, or should we just leave it as normal text?

Copy link
Member Author

Choose a reason for hiding this comment

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

@MayImilae, any thoughts?

const IOS::ES::TMDReader& tmd = m_volume.GetTMD(m_volume.GetGamePartition());
if (tmd.IsValid())
{
const std::string correct_ios = "IOS" + std::to_string(tmd.GetIOSId() & 0xFF) + "-";
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: Dash at the end to avoid partial matches. File names are "IOS‌ nn-xx-v vv.wad"

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, do you know what the xx stands for? It seems to always be 64, but I'm not sure why.

Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid not. Maybe 64-bit? No idea.

@JosJuice JosJuice force-pushed the verify-disc branch 2 times, most recently from 5d5d131 to 063543e Compare March 23, 2019 17:49
@JosJuice
Copy link
Member Author

This is what it looks like with hash calculation integrated:

image

@JosJuice JosJuice force-pushed the verify-disc branch 3 times, most recently from f374834 to 381c289 Compare March 30, 2019 18:55
@JosJuice JosJuice changed the title [WIP] Add a Verify tab to game properties Add a Verify tab to game properties Mar 30, 2019
@JosJuice
Copy link
Member Author

This PR should be feature complete now.

@JosJuice JosJuice force-pushed the verify-disc branch 3 times, most recently from b7b6dbc to 0c46441 Compare March 30, 2019 19:15
Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

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

Code looks good, untested tho.

Source/Core/DiscIO/DiscExtractor.cpp Outdated Show resolved Hide resolved
@JosJuice JosJuice force-pushed the verify-disc branch 2 times, most recently from 282e6a2 to 6b966ab Compare March 30, 2019 20:48
Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

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

It seems to be working for me. I tried it with my GC, Wii, WAD and exotic games such as DQX, SSBB.

I didn't have any issue with those. It also warned me with the ones I modded due to missing update partitions and other issues like missing files and so on.

The checksum calculation matches the one from redump regarding my games which didn't have verification problem.

Overall, LGTM.

@8times9
Copy link
Contributor

8times9 commented Apr 7, 2019

Could you set the window to not require scrolling for the tabs? On Windows the window is wide enough for everything except GameCube games.
Annotation 2019-04-07 150300

@JosJuice
Copy link
Member Author

JosJuice commented Apr 7, 2019

Hmm... Is there a good way to do that other than just making the window wider? And if we make it wider, should we make it wide enough for all languages or just some? In master, it's wide enough for English but not all other languages.

cc @spycrab

Source/Core/DiscIO/VolumeVerifier.cpp Outdated Show resolved Hide resolved
Source/Core/DiscIO/VolumeVerifier.cpp Outdated Show resolved Hide resolved
Source/Core/DiscIO/VolumeVerifier.cpp Outdated Show resolved Hide resolved
Source/Core/DiscIO/VolumeVerifier.cpp Outdated Show resolved Hide resolved
Source/Core/DiscIO/VolumeVerifier.cpp Outdated Show resolved Hide resolved
return Region::NTSC_U; // The most common country code for NTSC-U

if (revision)
{
if (*revision >= 0x30)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an appropriate upper bound that can be set here? I believe that a lot of homebrew likes to set funny revision numbers.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be possible to put some kind of limit on it, but there's no specific number that's the right one. I'm not especially concerned about this, since the intention behind the feature is that it lets you check if you have a correct dump or not, and homebrew releases are not correct dumps, so I'm okay with showing errors for them even if the description isn't quite accurate.

Copy link
Contributor

@spycrab spycrab left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

Source/Core/DolphinQt/Config/VerifyWidget.cpp Show resolved Hide resolved
Source/Core/DolphinQt/Config/VerifyWidget.cpp Outdated Show resolved Hide resolved
@JosJuice JosJuice merged commit d5ed3cb into dolphin-emu:master Apr 11, 2019
@JosJuice JosJuice deleted the verify-disc branch April 11, 2019 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants