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

solve hash collision in the mapping.json #589

Conversation

cengelha
Copy link

PR for issue #588. The goal is to have complete, easily accessible mappings for both, account data and saves.

To achieve this, two sets are created instead of one.

There are two possibilities for how to add them to the mapping.json:

  1. Currently implemented: Keeping the existing Mapping and concat the two created sets as lists. You can then split them before UserSettingsData but the list will have a handful duplicated entries (old 1164, this 1168).
  2. Replace the current Mapping with two separated lists (e.g. Mapping_save and Mapping_account)

@monkeyman192
Copy link
Owner

I think I prefer option 1 as you have it. I agree that it is a little strange having duplicates, however, a naive approach to implementing the usage of this file may read it in and convert it to a mapping of some type (eg. a dict), so these duplicates should disappear in this process.
Then, anyone who wants to make a better implementation can utilise the fact that the data actually has some structure (ie. the UserSettingsData marks the start of keys for that chunk of data.

Option 2 is not backward compatible so I think this is probably not a feasible solution.

It would be good to get @zencq 's input on this also to maybe get some insight into their implementation and whether this solution would fix zencq/NomNom#165

@cengelha
Copy link
Author

It's both me actually. Just some unusual account usage that tricked you I guess 😉

I extended my testing a bit and found that Version needs to be added for account too (therefore the new commit). With the new mapping.json I already have locally, everything gets mapped properly now and solves the issue 👍

I'm doing it as mentioned in #588 and by you: Looking for UserSettingsData and using everything before for save and including+after for account.

a naive approach to implementing the usage of this file may read it in and convert it to a mapping of some type (eg. a dict), so these duplicates should disappear in this process.

Exactly me thought!

@cengelha
Copy link
Author

Any plans to merge this any time soon? :)

@monkeyman192
Copy link
Owner

Any plans to merge this any time soon? :)

Sorry I have been on holiday...
I am happy to merge. When I get some time and I'm on my laptop I can check everything is fine and I'll merge it. Sorry for the delay!

@DjSheeno
Copy link

DjSheeno commented Jul 9, 2024

call_log.exml

@monkeyman192
Copy link
Owner

Sorry, I forgot about this... I'll merge now...

@monkeyman192 monkeyman192 merged commit d532cf5 into monkeyman192:development Jul 10, 2024
6 checks passed
@cengelha
Copy link
Author

All good. It's not critical and I had also temporarily forgot about it 😅 Just remembered it when I had to update it in some of my tests

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.

None yet

3 participants