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

Support headerless NES hashing in cheevos.c #7201

Merged
merged 1 commit into from
Sep 10, 2018

Conversation

rzumer
Copy link
Contributor

@rzumer rzumer commented Sep 10, 2018

Description

Attempts to hash a file in 16KB chunks when no NES header is present.
As far as I know only Mesen is able to load headerless files.

Reviewers

@leiradel

@rzumer rzumer changed the title Support headerless NES hashing in cheevs.c Support headerless NES hashing in cheevos.c Sep 10, 2018
@inactive123
Copy link
Contributor

@leiradel My bad, I merged this early. If you have any objections over this or if things need improving, you can suggest it here and the contributor can send a new PRs with the required changes.

@leiradel
Copy link
Contributor

This PR is unnecessary and doens't actually solve anything, and should be reverted:

  1. When the ROM is not recognized during the NES pass (it doesn't have the header, or it does but the hash wasn't found in the Retro Achievements database), cheevos.c will try the next heuristic which is a plain MD5 hash of the content.
  2. The official Retro Achievements NES emulator doesn't support the hashing that has been implemented in this PR, so there will never be a ROM with those hashes in the Retro Achievements database.

@rzumer
Copy link
Contributor Author

rzumer commented Sep 10, 2018

@leiradel, this change allows unheadered dumps to match existing MD5 hashes on compatible cores. It doesn't require adding new hashes to the database, and it is different from a full file hash. I can confirm that it works using the latest nightly (2018-09-10) without any additional changes.

In the event that there is an even number of CHR ROM blocks, the file size will be a multiple of 16KB and the final iteration of an unrecognized dump will be equivalent to the generic MD5 hashing method. We could attempt to prune a few iterations heuristically knowing the minimum number of CHR ROM blocks, but I don't think that it is necessary, considering the low computational cost of these few extra MD5 hashes.

@leiradel
Copy link
Contributor

I've talked to @rzumer and he explained to me that his method will produce the same hashes for headerless ROMs as the current method for ROMs with headers, which makes RetroArch support achievements for headerless ROMs even though the official Retro Achievements emulator doesn't.

This PR is good and can stay. Sorry for jumping to conclusions, I haven't realized the hashes wound be the same for the same ROM with and without a header.

@andres-asm
Copy link
Contributor

andres-asm commented Oct 1, 2018

All the NES cores can load header less ROMs BTW.

@leiradel
Copy link
Contributor

leiradel commented Oct 1, 2018

All the NES cores can load header less ROMs BTW.

Yes, it's not an issue with the cores but with recognizing headerless NES ROMs at RetroAchievements. I believe this should be done by adding the additional hashes to their database instead of making multiple HTTP requests from RetroArch.

@rzumer
Copy link
Contributor Author

rzumer commented Oct 1, 2018

I believe this should be done by adding the additional hashes to their database instead of making multiple HTTP requests from RetroArch.

Agreed, but as we don't have the tooling to get this done right now, this is sufficient (with #7333 fixes) as a stopgap until the database can be cleaned up and moved towards full content (excluding header) hashing.

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

Successfully merging this pull request may close these issues.

4 participants