Closed fzzyhmstrs closed 2 months ago
Also general comment re: var
. You allowed several through previous review, so this critique is a fix of code previous to my work.
Four comments I've left still unresolved; otherwise should be buttoned up now pending another look. Once I have a bit of feedback I should be able to resolve everything open easily.
/////
As a side note to the PR, I find some of the documentation of API particulars to be less than ideal. SlotAttributes#addSlotModifier
for example. I did not understand the implications of the method from the documentation (does the operation arg do anything? Does the identifier do anything? At first I actually didn't understand what it was even doing, until I read the wiki):
TrinketDropCallback#drop
(particularly what each choice of the enum does) and TrinketApi#registerTrinketPredicate
(what behavior does TriState.DEFAULT provide? when should I use it over FALSE?) are some others that come to mind.
I bring it up because I would be happy to add some docs in the PR, but it's your documentation for your implementation details, so I am hesitant. Also at this point I'd probably just rather see the release happen than me come up with more stuff to do.
My current thought re: the methods we aren't happy with is to add a TrinketModifiers
class to the top-level package with the two trinketModifiers
static methods and the toSlotReferencedModifier
from SlotAttributes
.
Probably rename the trinketModifiers
methods to something simpler, like TrinketModifiers#get(args)
If you are amenable to the idea I'll whip it up
The API/impl split of trinkets (and the general pattern for all of my projects) is everything in dev.emi.trinkets.api
is API, everything else is impl. Re the general critiques about code style: a lot of style against my general preferences has leaked in over time, I'm only concerned with the stuff you're adding, anything else is my responsibility to go clean up some time. I think a top level TrinketsModifiers
class with all the static helper stuff you've designed is reasonable to take it out of API space. Thanks for sticking with me in the review process.
Everything should be buttoned up in the latest commit. The issue of the NBT modifier behavior remains.
re; the unresolved conversation above, I tested adding a unique
field to the TrinketsAttributeModifiersComponent
. By default this flag is true
, which uses vanilla behavior, and leaves the id untouched. The user can set unique:false
to have a modifier that works in multiples in either the same slot or various slots.
Only issue is, this would be undocumented behavior compared to vanilla attributes.
I think I'm going to maintain here that the attributes should have parity with vanilla, and the uniqueness strategies they have. If they've changed it from how they do uuids then that can be how trinkets does it.
Reverted attribute behavior to vanilla style, removing the helper method.
Figured out some new things about intellij to make it stop doing star imports and space-tabs. Neat.
Hopefully all buttoned up now.
Resolves:
Explanation
👍 Added new
TrinketEquipCallback
andTrinketUnequipCallback
. Each have the same parameters as theTrinket
method itself. They are wired in the same spot as the Trinket method, directly after the trinkets method is called. Added simple test registrations for them; both working.👍Added new method
getRefId()
to SlotReference; tucking away the messy inventory().slotType().getId() etc etc.👍Used this new method in a new method in
SlotAttributes
;toSlotReferencedModifier
. This method suffixes the modifier with thegetRefId()
. This avoids collisions if multiple slots are available for one "slot", or if trinkets could go into multiple/any slot, and attributes are being added via NBT. Previously the identifier added via NBT would be used directly; causing conflicts if 2+ of the same NBT trinket are equipped.👍Incorporated this new method into a new static method in![image](https://github.com/emilyploszaj/trinkets/assets/72876796/9769383d-cc89-4a62-8557-a42ec48f37a6)
Trinket
;trinketModifiers
. This flips the script on the previousBuild NBT Attributes -> Hope the user calls super -> return map
. Now the flow isProvide blank multimap if the user calls super -> user adds code-side modifiers -> Trinket adds NBT modifiers -> return map
. This flow is tested in the new TestTrinket2, which does NOT call super, and yet both modifiers are still apparent (presuming you/give
yourself a TestTrinket2 with some NBT modifiers).Also encapsulates
getTrinket
andSlotAttributes.getIdentifier
, as at every Trinkets call site those were simply generated immediately before passing them in. Now they are just generated in situ, with an overload available to be custom if needed.Technically a implementation change in the off chance that someone was relying on finding an NBT attribute to determine if they do anything attribute related in code (despite your insistence in the docs to keep the contract pure). Unsure the impact compared to people just not calling super (definitely happens). Understandable if you want to flip this logic back, with the other improvements intact.
👍Wired the new
trinketModifiers
method into all the Trinkets call sites.👍Polished
ItemStackMixin
:hand/ring
andoffhand/ring
. Reason this was still happening: Equality ofSlotType
was checked with the translation, but the relevant maps were then stored with theSlotType
as the key, undoing this check. Map now uses theMutableText
that was checked as the key directly. Results:Before:![image](https://github.com/emilyploszaj/trinkets/assets/72876796/ecfb7f73-8a3d-4d1b-96f4-933064609c48)
After (Test Trinket 2 is equippable in both ring slots):![image](https://github.com/emilyploszaj/trinkets/assets/72876796/a965bd2f-8a18-41e9-a047-e1e5fda816ed)
👍Added
clear
argument to the trinkets command (/trinkets clear
). This clears out all of the trinket slots.Could Do
Could add
canEquip
andcanUnequip
callbacks to have Event functionality for those main 4 trinket methods. I stopped atonEquip
andonUnequip
because that was the asked content in the issue, and custom predicates can be made.Recommend
Adding info about the two new events to the Wiki