Curio stacks validation order and method
Aizistral opened this issue · 4 comments
Please describe the new feature or change.
I suggest two changes to be made into how curios are loaded and validated. First - don't try to validate them before we're done with all that reading-from-NBT action and they actually are within their slots. Second - create separate method in ICurio and thus ICurioItem, specifically used to check whether or not the item can persist in it's slot when it's already there, named something like canPersist or isValid.
Please describe why you want this feature.
First, let's take a look at this:
Apparently, what Curios does is that it serializes a stack from NBT data, checks if that stack can be equipped as if it wasn't previously, if so - let it persist in it's slot, otherwise - eject it. Then proceed to next stack, so on until we're done reading them all. Only after we're done with them all - set newly constructed curio handler for player in question.
The problem is that it asks each stack about whether or not it could be equipped before we have all these stacks in place within original inventory. This does not allow to check the curios inventory state when deciding whether or not certain curio should be allowed to be equipped. Why is this an issue? Well, for instance - I have certain accessories in my mod that can only be equipped if other accesories are present, mainly these are items that require Ring of the Seven Curses, since that ring is a core of one of the crucial mechanics in Enigmatic Legacy. Needless to say, all such items are getting ejected when re-logging into the world, because they can't find RotSC they require to be equipped, because new handler is not in place yet, and it may not even be serialized at the moment.
In regard to the second suggestion, it will make validation behavior more consistent and give implementers better control over it. Even if we validate all curios after we're done loading curio inventory capability, there might be other capabilities which are not loaded at the time. Plus - I myself had canEquip conditions in my mod that stop returning true once the item actually ends up being equipped, mainly to prevent equipping duplicate curios. I would never have expected this condition will be checked when items are already equipped, and I doubt anyone really would, until they encounter some problems with this behavior like I did.
Fair points. This should be easy enough to implement, so I'll try to get to this soon. Sorry for the troubles this caused in regards to your mod and its interactions with the canEquip method.
I see, well I look forward to their fix! In the mean time I've asked my server members to maintain sufficient inventory space so that they don't lose their items.
I decided to take a softer approach to this issue and instead just addressed the direct cause.
The reason this occurred is because, during the last update, I tried to consolidate validation into a single method ICuriosHelper#isStackValid in an effort to provide one source of truth for validity. However, my main mistake was merging together the behavior for equipping items and validating items, which are not necessarily the same thing as outlined in this issue. Therefore, my solution was to simply keep most of the code intact but separate the ICurio#canEquip behavior out so that it no longer attempts to use that method as a form of deterministic validation. This should result in behavior that is consistent with behavior before 4.0.5.0 but with better support for datapack changes.
I've also personally tested this with Enigmatic Legacy's mechanics and can confirm the ejection-on-load behavior is fixed (unless I missed some specific behavior about it).
There are some lingering concerns I have about whether or not the current state allows modders enough control over validation, which may be fixed by providing the aforementioned isValid method, but it would involve enough consideration and refactoring that I've decided to tackle that at a later date when a specific use-case for it is brought up to me.