search

wisp-forest / accessories

A extendable and data-driven Accessory Mod for Minecraft
Other
29 stars 10 forks source link

[Feedback] SlotReference API methods and Desires for API #73

Open fzzyhmstrs opened 3 months ago

fzzyhmstrs commented 3 months ago

I have some issues with various methods in the AccessoriesAPI, Namely the ones like getAttributes that use SlotReference.

There are 6 or so methods in the API that are called with SlotReference parameters. My issue with this is, SlotReference seems to be effectively internal, with visibility in events. There are a lot of questions around SlotReference usage from a third party perspective in a vacuum (not in an event).

  1. Is it correct for a third party to create a Slot Reference as-needed?
  2. If so... how does the third party handle the index param? There might be 0 of the slot, or 100, what is the proper way to handle this? This is the crux of my issues with a lot of the API calls.
  3. If the 3rd Party is not intended to make their own SR, how do you acquire them? How do you use any of the API methods without easy access to the reference?
  4. If there is a method for either 2 or 3, it is not very intuitive or visible (at least to me), and should probably be directly front and center in the API (for use elsewhere in the API).

Cue ramblings below

Personally I would like more 10,000 foot API calls that don't require any inner knowledge.

  1. Access to functionality of AccessoriesCapability without having to separately know of it and work with it. At least a centralized get that doesn't require knowing you have to get it directly from AccessoriesCapability itself.Having to work within the capability feels a bit like having to directly work in the CCA component in trinkets.
  2. canEquip, canUnequip, isEquipped calls that don't require a SR. Pending the issue highlighted above, there isn't really a good API-surface way to check things like this. As discussed in the discord, the only way seems to be via the capability, with actually equipping it and checking the result. Don't know about unequipping. canEquip(ItemStack stack, LivingEntity entity) or canEquip(ItemStack stack, LivingEntity entity, String slotName) are two examples of easy to use method signatures that don't require knowing specific runtime state like index count. Just... "can it go in?".
  3. getPredicateResults is another candidate. I personally have no idea what a third party would do with these methods from an API point of view, but regardless of that, even the implementations elsewhere in the API hint at a potential shortcut method signature that doesn't require handling runtime internal state like the set of identifiers.

getAttributeModifiers tries to do what I'm talking about with various simpler calls, but still falls into the "I have no clue what slot index to care about" trap.

To me the presence of internal, dynamic state (slot indexes, namely) is an indication that some of these API methods are really internal, unless that 10K view signature is possible. I wouldn't know how to get out of the index issue on the attribute modifiers calls, since those are really just hiding the need to make a SlotReference. Unlike the equip methods I'm not sure how I'd get out of it for attributes, which are being retrieved for an exact slot.

Dragon-Seeker commented 3 months ago

Thanks for the issue as I do care to improve the API where possible and below is a response to things talked about:

  1. SlotReference is not designed to be created by others but is possible depending on the situation, so it's mainly there to show public API that the end user has access to I guess.

  2. The index parameter must be within the bounds of the given AccessoriesContainer which can be gotten using the references slotName (Must be the string name as SlotType is reloadable so having a hard reference could be a problem.) It requires getting the LivingEntity's current container instance and using the size to properly iterate though the containers entries and/or making sure you have a valid index

Note: I have plans within the future to make SlotType and relative entity type bindings into a registry of some type as then I can pass the info to RegistryAccess (Yarn: DynamicRegistryManager) but I have yet to investigate such.

  1. The idea of needing to create a SlotReference is if you know what your doing type deal, but there are various methods you may use to do such if you are adding support for Accessory system. AccessoriesContainer#createReference is helpful if you are iterating though all containers as it only requires the index to be passed to make a reference meaning all you need to do is if you need to iterate though containers entries just doing AccessoriesContainer#getSize with a for i loop allows for making easy references.

Overall starts my initial thinking of reworking AccessoriesAPI which I will discuss later here. I believe that the current state of SlotReference is designed around it being passed to third parties with the idea of if you know what you are doing you can make such if required for your application.

Comments on: Cue ramblings below section

  1. My main goal of AccessoriesCapability is to contain methods for bulk interaction with containers with such being the ability to check all containers for a ItemStack/Item, modifying any of the LivingEntity's slot sizing though the use of SlotAttributes, easy equipability checking and handling, combined with proper validated access to various context objects required to perform these actions if needed, like the LivingEntity and AccessoriesHolder. This object is not the CCA component in a sense as such would be the AccessoriesHolder as it holds onto the containers and various other data objects and is the public facing API implementation. I believe proper documentation over the various core objects and methods within Accessories would help fix this issue than adding to AccessoriesAPI. Furthermore, I have expected many to use the Interface injection that is present on LivingEntity to access this class though I have found only good support for Fabric and Arch Loom with such requring modImplementation on common for it to work which is honestly stupid. I hope to get native support for NeoForge with their version of Interface Injection combined with investigation onto other multiloader project templates as I think this is the proper way of handling combined with previous stated info about such.

  2. Already have started reworking the current AccessoriesCapability#equipAccessory method as it differs to how vanilla inventory interactions work which may lead to possible issues combined with lack lust ability to optionally check and then perform an equip with other steps to prevent issues as discussed within Dev Dungeons Discord. I have redesigned to have a attemptToEquipAccessory and canEquipAccessory method with ItemStack for the given stack to be attempted to equip/check for equipability combined with a boolean value for indicating if an attempt to swap out stacks is allowed or not. Overall I don't quite get where and why you would need a canUnequip where you do not have or can not make a SlotReference but have a possible ItemStack that is equipped... like it seems more you have not passed the proper SlotReference far enough down. I also believe the canEquip(ItemStack stack, LivingEntity entity, String slotName) is honestly quite concerning due to that hard coding where it can be equipped within the code though the passed String slotName rather than though predicate system and the Accessory#canEquip method which is seems unneeded and frankly something I want to avoid people doing in general to allow for end user as much customizability at all costs unless the dev uses the various methods available to lock such down for reasons very intentionally.

  3. Honestly another one of those if you know what your doing use it methods that is really only designed to be used for specific checks within Accessories. It's mainly used for other methods to get the required predicates for slot validation which are more user focused.

What this has made me rethink is breaking up AccessoriesAPI instead as it serves a purpose of showing all public available API but frankly doesn't make any sense when to is just a mess of various methods without much purpose being together. All in all I believe that their better off being in respected areas with proper documentation combined with great information about the API the Accessories provides. Furthermore, some methods may be called by some but most will never need to touch such whatsoever leading them to be more for internal use rather than publicly used methods.