-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Small fix, some new functionality and localization update #72
Small fix, some new functionality and localization update #72
Conversation
My IDE also automatically removed unneccessary (double) casts, not something I intended but don't see a reason to keep them anyways
Also automatically removed unnecessary casts and added 'this.' qualifiers to where they were missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some really good stuff here, just a couple concerns up for discussion.
src/main/java/top/theillusivec4/curios/common/event/CuriosEventHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/top/theillusivec4/curios/common/objects/FortuneBonusModifier.java
Outdated
Show resolved
Hide resolved
Fair point. Will push changes as soon as I get back to the computer. I wonder if it makes sense to collect and store data on fortune/looting bonuses when entity's curios are cycled through in tick handler, rather than needlessly repeat that code and cycle through all stuff all over again for events when these values are needed. |
Anyhow, I altered the implementation so that Fortune and Looting bonuses are stored in inventory wrapper, being recalculated in each LivingUpdateEvent along with other stuff that gets handled when cycling through curios map. This also allows to externally check (and even alter, if for whatever reason needed) values of those bonuses through an API, which I hope is good thing. |
So, list of changes:
Fixes:
Features:
ICurio#showAttributesTooltip
call that kindly asks each curio whether or not tooltip listing their attributes when worn should be added, for each identifier. Sometimes mod creators might want to outline those attributes as part of their own stylized tooltip, similar to how I often do. I noticed"HideFlags"
nbt check inItemTooltipEvent
listener which in theory may allow to achieve this, but it seems to be an inherited vanilla behaviour that isn't very obvious and I guess affects more than just Curios attribute tooltips.Added
curios:equip_curio
criterion trigger, which is fired in same places whereICurio#onEquip
calls are dispatched. Current implementation allows to perform item and/or location tests if neccessary. Might prove useful for some custom advancements thingies.Added compatibility with Fortune and Looting enchantments. Default implementation considers enchantment levels of these enchantments applied to each curio
ItemStack
, if there are any, then applying total Fortune bonus through specialGlobalLootModifierSerializer
and Looting throughLootingLevelEvent
, respectively. Of course wearables cannot legitimately receive those enchantments by default, but mod creators can make it possible for their individual trinkets I suppose. Furthermore, default implementations reside inICurio
, as a way to alter or completely override amount of provided enchantment levels for both Looting and Fortune bonuses.This functionality can surely be achieved by each mod on their own, but I assume many mod creators may want to add curios that simply increase Fortune and/or Looting traits when worn, so why not spare us a burden of having to reinvent and reimplement this wheel.
Localization:
If you aim for parity between Forge and Fabric versions, it might be fair to somehow implement this stuff on Fabric port either, but I unfortunately can't help with that as so far I still have no idea what Fabric is even about.