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

Make netplay's "same game" check more robust #8861

Merged
merged 3 commits into from Sep 6, 2020

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Jun 10, 2020

Instead of comparing the game ID, revision, disc number and name, we can compare a hash of important parts of the disc including all the aforementioned data but also additional data such as the FST. The primary reason why I'm making this change is to let us catch more desyncs before they happen, but this should also fix https://bugs.dolphin-emu.org/issues/12115. As a UX bonus, the UI can now distinguish the case where a client doesn't have the game at all from the case where a client has the wrong version of the game, and we can show translated/custom names in the netplay game list.

@JMC47
Copy link
Contributor

JMC47 commented Jun 10, 2020

This is really nice actually. Huge thumbs up.

// 2. Different for discs with differences that affect netplay/TAS sync
// 3. Much faster than hashing the entire disc
// The way the hash is calculated may change with updates to Dolphin.
std::array<u8, 20> sync_hash;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps an alias like:

using SyncHash = std::array<u8, 20>;

should be made (or encapsulate it in its own struct if forward declaring matters), given how often this is repeated throughout the code to make it more self-documenting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. However, I haven't made use of this alias in DiscIO, since it feels a bit silly to include SyncIdentifier.h only for the alias. Do you think I should use it in DiscIO anyway?

Copy link
Member

Choose a reason for hiding this comment

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

I was mainly suggesting it assuming it would be used consistently, so if it isn't going to be used like that, then there's no real point of using the alias.

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'll revert the change, then.

@JosJuice JosJuice force-pushed the netplay-hash branch 2 times, most recently from 005ab68 to 48bbc8a Compare June 10, 2020 18:01
@AdmiralCurtiss
Copy link
Contributor

Something went wrong with your rebase here.

Instead of comparing the game ID, revision, disc number and name,
we can compare a hash of important parts of the disc including
all the aforementioned data but also additional data such as the
FST. The primary reason why I'm making this change is to let us
catch more desyncs before they happen, but this should also fix
https://bugs.dolphin-emu.org/issues/12115. As a bonus, the UI can
now distinguish the case where a client doesn't have the game at
all from the case where a client has the wrong version of the game.
There is no longer any major reason for why this function would
need to return the same result for all players.
The new hash check catches essentially all desync problems
that VolumeVerifier can catch, so from the user's perspective,
such problems will result in Dolphin refusing to start the
game on netplay rather than actually getting a desync.
@JMC47 JMC47 merged commit e7e5175 into dolphin-emu:master Sep 6, 2020
10 checks passed
@JosJuice JosJuice deleted the netplay-hash branch September 6, 2020 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants