Skip to content

Conversation

@dgelessus
Copy link
Contributor

@dgelessus dgelessus commented Mar 2, 2025

In the current code, the ==, !=, and < operators for plLocation all look at different combinations of fields. In particular, == doesn't look at fFlags even though != does, and < doesn't look at fFlags or fState. This PR makes all three operators consistent with each other.

These buggy implementations led to some bad behavior when editing a PRP's flags in PrpShop, which seems to be fixed with this PR. This hopefully fixes the problems that @TheScarFr had when trying to change a PRP from reserved to non-reserved (I'm not completely sure, because I couldn't reproduce the bug exactly).

Apparently the buggy code has been this way for over 15 years? This seems really bad! I'm surprised that this hasn't caused any other problems with PRP edits before.

While I was here, I also replaced all operator!= implementations with return !operator==(other); to prevent any future inconsistencies between == and !=, and added operator!= overloads to classes that only defined operator==.

@dgelessus
Copy link
Contributor Author

Unfortunately, this change currently causes other issues, leading to PrpShop crashes in some cases...

The plAgeInfo methods that translate page indices to plLocations return locations with no flags, regardless of the actual correct location flags for the page in question. This causes problems when using those locations elsewhere - e. g. plResManager::FindPage will now fail to find the page in question if the flags don't match the page.

In particular, plAgeInfo::getCommonPageLoc now always returns incorrect locations (because common pages should always have the kBuiltIn flag set), and all of the methods now return incorrect locations for global pages (which should always have the kReserved flag set). Unfortunately, we cannot just make these methods set the correct flags, because not all of the flags can be predicted from other properties of the page - in particular, the kItinerant flag (used by Garden_District_ItinerantBugCloud).

I'm not sure what's the best way to fix this... It feels like we may need two different kinds of equality comparisons for locations: one to check if two locations are exactly the same or not (looking at all fields, including flags) and one to check if two locations conceptually refer to the same page (looking only at the state and age/page numbers). This would probably require changes to a lot of code that uses libHSPlasma, which is not great, but may be the safest move considering the previous inconsistent behavior...

@dgelessus
Copy link
Contributor Author

Actually, it looks like the affected plAgeInfo methods are only used in a handful of places (getPageLoc, getCommonPageLoc, getPageLocs), so perhaps it would be smarter to deprecate those methods and encourage looking up pages by name instead. This would sidestep the problems with "half-matching" locations, because two age/page names are always either clearly equal or not.

Fixes some odd behavior in PrpShop when changing a PRP file's flags.
Most likely this also caused other weirdness besides that.
To prevent any future inconsistencies between the two.
@dgelessus dgelessus force-pushed the fix_equality_operators branch from 93bb3e3 to 26e57fc Compare April 13, 2025 20:39
Otherwise the plAgeInfo methods getPageLoc, getCommonPageLoc, and
getPageLocs can no longer be used, because they return locations whose
flags do not necessarily match the flags of the real corresponding pages
(and those methods *cannot* be fixed to return the correct flags in the
general case, because of the kItinerant flag, which cannot be predicted
from other properties of the page).

This fixes the crash when opening an .age file in PrpShop, caused by the
preceding commits.
@dgelessus
Copy link
Contributor Author

I've now implemented the first fix, by adding a new method that implements the previous behavior of ==, and adjusting a few obvious places in libHSPlasma to use that method (namely FindPage and UnloadPage).

This fixes the PrpShop crashes I encountered, but I'm not confident that this is a complete fix... Various other code still relies on the default comparison operators of plLocation (e. g. plKeyCollector's keymap_t) and I have no idea if that might have similar issues.

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.

1 participant