Unify plain and type of for items#8402
Unify plain and type of for items#8402sovdeeth wants to merge 6 commits intoSkriptLang:dev/patchfrom
Conversation
Efnilite
left a comment
There was a problem hiding this comment.
looks good, could use a plain item test
plain is already tested in quite a few places throughout the tests, but if you think a standalone is worthwhile i can do that too |
b099319 to
4c40868
Compare
There was a problem hiding this comment.
So, there are certainly some breaking changes to note with the changes to plain. I'm not too worried about those, as they are more intuitive. My main concern is that with these changes, there is seemingly no way to obtain the base representation of any item (e.g. just the item created from a material). This system was always kind of bandaged together waiting for a proper comparison system to be implemented. Whenever that may be, I agree some changes now could provide further relief. If plain isn't a true base item (which I can understand the reason for), I think type of should be. It is a useful comparison option/tool to have, so I don't think we should remove it outright.
Requesting changes to enforce further discussion.
I agree that there isn't a material-only method for everything, but i'm not sure that's a big deal. This only affects potions and goat horns, as far as I know. In both cases, the base item isn't actually something accessible in game, normally, and with both, I'm not opposed to type of being the base item, but I do think it might cause more breaking changes than are specifically needed, while not really adding all that much benefit. Let me know what you think |
APickledWalrus
left a comment
There was a problem hiding this comment.
Fair enough. This should be fine for now
Problem
type ofhas been misleading for users when used with items, as it only clears names and durability, rather than just returning a completely base item representative of the type.plaincan return weird items when used with things that require ItemMeta to differentiate, like potions (returns uncraftable potion) and goat horns (always returns ponder).Solution
Changes both expressions to use a new
getBaseTypemethod on ItemType that replacesaliasCopy. This copies over only the minimum necessary info (potion meta info and goat horn info) to differentiate items. This meansplain strength potionnow actually returns a strength potion, andtype of diamond sword of mendingreturns a diamond sword without enchants.I don't expect this to break anything for the user, but I'm not entirely sure it is 100% safe. Testers are welcome! I've done testing around potions and goat horns and enchantments, and it seems fine to me, but I'm not sure if there's some edge case I'm missing.
Testing Completed
regression test, manual testing
Supporting Information
There's a second option here, where
type ofalways returns the item with no other identifying data. This may be more desirable for thetype ofcould be interpreted as the material alone. However, this meanstype of strength potionisuncraftable potion, andtype of strength potion is type of speed potionreturns true. I don't think these changes are intuitive and could be breaking for users. I am open to counter-arguments, though.Completes: #8144
Related: none
AI assistance: none