H-uru / libhsplasma

Cross-platform Plasma data and network library
GNU General Public License v3.0
32 stars 30 forks source link

Fix incorrect keyring flag. #233

Closed Hoikas closed 2 years ago

Hoikas commented 3 years ago

Legacy MOUL clients expect for the keyring to be sorted by name. If keys are not sorted and the flag is not applied, then searches by name will silently fail. This was observed when someone attempted to add a new image to GUI_District_BkBookImages.prp and use it a journal. Due to this issue, no image was returned by the journal's search.

zrax commented 3 years ago

If searching is more efficient with a sorted keyring, perhaps we should sort the keyring instead...

Hoikas commented 3 years ago

Perhaps, but that codepath was actually removed from H'uru/Plasma, so we'd be writing more code to support a dead-end (MOULa). I'm also wary of introducing another case similar to #218.

zrax commented 3 years ago

True, but we also support several other dead-end games, and this code is in a block that would also affect Myst 5, for example...

Perhaps instead of trying to automatically sort or blindly writing a flag that says it's unsorted, we could keep track of whether the keyring is unsorted (simply setting it unsorted when adding new keys is sufficient -- we can then add another method to sort it which clears the flag some time later if necessary).

Hoikas commented 3 years ago

That's a good point. To keep the code simple, I wonder if it would make sense to make the optimized sort an explicit step that needs to be done before writing out a page? Or would you prefer to maintain the key list in its optimized form at all times? My concern with doing that is that there will be more potential for keys to be re-numbered when doing seemingly trivial operations.

Edit: I see you added an answer to this question as an edit. Whoops lol. I'll implement it like you suggest soon.

dpogue commented 3 years ago

My concern with doing that is that there will be more potential for keys to be re-numbered when doing seemingly trivial operations.

After #218 aren't the keys always re-numbered when doing any writing at all? If that's the case, it doesn't seem like it should be too difficult to sort them while re-numbering them

Hoikas commented 3 years ago

218 changed it to re-number the keys on read. Previously, we were re-numbering them on write, which would silently break the world. Note that's re-number, as in, make the values contiguous, starting at 1. For this keyring flag to be zero, we need to case-insensitve sort the keys, which is an even more fun operation ;)

Hoikas commented 2 years ago

Ok, I've added sort-by-name as an opt-in feature with mitigation for old (read: broken) files produced by previous revisions of libHSPlasma.