-
Notifications
You must be signed in to change notification settings - Fork 438
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
Fabric Component Access #1067
Fabric Component Access #1067
Conversation
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.
Good start, there is a lot we still need to cover to get this ready.
There is a lot of useful documentation in the PR description that could probably work well as code examples in javadoc to guide experienced users who are approaching this system having generally never used it before.
...access-api-v1/src/main/java/net/fabricmc/fabric/api/component/access/v1/ComponentAccess.java
Outdated
Show resolved
Hide resolved
* | ||
* @return The player holding the item in which the component resides | ||
*/ | ||
/* @Nullable */ ServerPlayerEntity player(); |
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.
Is there a reason this couldn't just be an Entity?
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.
Hmmm, you mean like if a Zombie is holding an item?
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.
Yea
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.
The player version is specifically for items held by a player. For a Zombie, you'd provide a StackGetter/StackSetter functions that know which zombie (or other entity) and how to access the item stack in it.
We could add more specific handlers for mob-held items, items in an item frame, items floating in the world, etc, but where to stop?
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.
I think the concept of equipment is introduced to entities at around living entity, so that should be a fine place to go down to.
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.
I'll need to think about it, but we could probably expand Player to be Living Entity. Will still need the variant that accept a null Entity for things like ItemFrame items, ItemEntity, etc.
...access-api-v1/src/main/java/net/fabricmc/fabric/impl/component/access/ComponentTypeImpl.java
Outdated
Show resolved
Hide resolved
...access-api-v1/src/main/java/net/fabricmc/fabric/impl/component/access/ComponentTypeImpl.java
Outdated
Show resolved
Hide resolved
...pi-v1/src/main/java/net/fabricmc/fabric/impl/component/access/ComponentTypeRegistryImpl.java
Outdated
Show resolved
Hide resolved
...t-access-api-v1/src/main/java/net/fabricmc/fabric/api/component/access/v1/ComponentType.java
Outdated
Show resolved
Hide resolved
...v1/src/main/java/net/fabricmc/fabric/impl/component/access/AbstractComponentContextImpl.java
Outdated
Show resolved
Hide resolved
...pi-v1/src/main/java/net/fabricmc/fabric/impl/component/access/ComponentTypeRegistryImpl.java
Outdated
Show resolved
Hide resolved
...pi-v1/src/main/java/net/fabricmc/fabric/impl/component/access/BlockComponentContextImpl.java
Show resolved
Hide resolved
...api-v1/src/main/java/net/fabricmc/fabric/impl/component/access/ItemComponentContextImpl.java
Show resolved
Hide resolved
…ic/impl/component/access/ComponentTypeImpl.java Co-authored-by: i509VCB <i509vcb@gmail.com>
Hmm, thought about this a bit, how much work would it be to make a custom ComponentAccess refer to a unique source such as the abyss for example. I would like to consider working in a way where that would be somewhat possible. |
I don't understand what this means. Could you elaborate? |
naming, final, use DFU pair, simplify error check
…fabric into fabric-component-access
...-api-v1/src/main/java/net/fabricmc/fabric/api/component/access/v1/BlockComponentContext.java
Outdated
Show resolved
Hide resolved
...access-api-v1/src/main/java/net/fabricmc/fabric/api/component/access/v1/ComponentAccess.java
Outdated
Show resolved
Hide resolved
...t-access-api-v1/src/main/java/net/fabricmc/fabric/api/component/access/v1/ComponentType.java
Outdated
Show resolved
Hide resolved
...t-access-api-v1/src/main/java/net/fabricmc/fabric/api/component/access/v1/ComponentType.java
Outdated
Show resolved
Hide resolved
...t-access-api-v1/src/main/java/net/fabricmc/fabric/api/component/access/v1/ComponentType.java
Outdated
Show resolved
Hide resolved
...api-v1/src/main/java/net/fabricmc/fabric/api/component/access/v1/EntityComponentContext.java
Outdated
Show resolved
Hide resolved
...pi-v1/src/main/java/net/fabricmc/fabric/impl/component/access/BlockComponentContextImpl.java
Show resolved
Hide resolved
* | ||
* @return Position of the block component within the world. | ||
*/ | ||
BlockPos pos(); |
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.
I'm fine with both, but I'm guessing get prefixes are more common throughout FAPI.
private final Function<ItemComponentContext, ?> defaultItemMapping; | ||
private final Function<EntityComponentContext, ?> defaultEntityMapping; | ||
|
||
private final IdentityHashMap<Block, Function<BlockComponentContext, ?>> blockMappings = new IdentityHashMap<>(); |
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.
IdentityHashMap
does not technically fulfill the Map contract, so I'd say it's fine to declare it differently.
...access-api-v1/src/main/java/net/fabricmc/fabric/impl/component/access/ComponentTypeImpl.java
Show resolved
Hide resolved
* @param mapping mapping function that derives a component instance from an access context | ||
* @param predicate Mapping will apply to all entity types that match this test | ||
*/ | ||
void registerEntityProvider(Function<EntityComponentContext, T> mapping, Predicate<EntityType<?>> predicate); |
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.
This will not work. EntityType
s do not have a reference to a Class
(much like ComponentType right now), so mods cannot test whether the entities of this type implement something or not.
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.
I had not thought to interrogate the entity classes here, but I can see how that would be useful for inferring the presence of an interface. Would probably be better to accept both EntityType and the Entity class. I don't think passing an Entity instance is desirable.
Would that be acceptable?
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.
Indeed, I believe Entity class is all that's needed for most purposes.
@SuppressWarnings("unchecked") | ||
@Override | ||
public <E extends Entity> ComponentAccess<T> getAccess(E entity) { | ||
applyDeferredEntityRegistrations(); |
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.
This is a bit fragile, mods could create dummy entities during init.
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.
Just to confirm, you mean they may call this during init with a transient Entity instance before registration is final. Is that right?
Does Fabric yet have an event or other supported means to defer this until registries are final? I'd have to look.
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.
That's indeed what I thought about. I don't know if there is a better way that doesn't involve entrypoints though.
I think it is too early to jump into the review, there are some fundamental design aspects to be discussed. For me the problem space (not the particular PR) may have the following features:
This PR approximately handles 1.-4. The features 5-6 have no business being directly coupled to it, however it needs to be possible to link them for efficiency reasons (store the bound API object with the data -> save some steps). 3 and 4 are very expensive and needlessly mandatory. 3 in particular is quite weirdly implemented in that Direction has been integrated as an universal concept, while it is only applicable to blocks. It also assumes the Direction and a random Identifier are universally suitable to retrieve the fully bound API access. I believe this is better done by splitting 1+2 from 3+4, making the latter optional and fully customizable. 1 can be some sort of type to (usually) singleton binding like 2 would be For 3+4 I'd at most offer a couple free-standing base classes similar to your *ContextImpl that store the relevant interaction parameters and offer the actual API via a sub class[3]. It'd be easy to instead use a custom class that asks for more appropriate application specific context. In the above example the uses of MyApi become MyApiProvider, offering a method I'm still unsure whether the use of such unifying wrappers should be encouraged due to the overhead. APIs can usually work around the lack of unification by providing overloads, most of the time the caller is in a specific context already and for generic APIs the other side uses some prefabricated implementation. I did not understand at all what those "Actions" that can somehow be applied are supposed to be. The various acceptIfPresent and applyIfPresent overloads are IMO rather pointless. Not only are they very likely to allocate via capturing lambdas and boxing, but also pollute the API and don't fit that nicely in the call sites I remember. [1] Using instances of Block or Identifiers there is closer of "types" in the logical sense, not Java types. The Java types (classes) overloads register to all instances. [2] ApiProvider is just for faster lookup and type safety. [3] AFAIK inheritance is the only option that is both as efficient as possible and offers a clean API. There is nothing preventing other choices though, like yielding a context object to pass to methods in a separate class or allocating a separate object. [3] |
Replaced by #1072 Closing this because the changes are too extensive for pleasant reviewing. |
Component Provider
A component provider is a functional role - there is no
ComponentProvider
interface or class. Component providers are game objects (currently blocks, items or entities) associated with one or more "Component Type" instances. Components are also conceptual and can be of any type - there is noComponent
interface.Many readers will note some resemblance here to entity attribute frameworks like Cardinal Components or the old capabilities system in Forge. The resemblance is only approximate: this API facilitates access to components but handles no aspects of implementation or persistence. It solves the problem of "how do I get the thing from the other thing when I don't know how the first thing is implemented in the other thing" problem.
This API is in no way a general-purpose entity attribute library, nor is it meant to replace one. The intent here is to decouple component access from component implementation, and to do so without adding or requiring external dependences. It should be complimentary to CCA, LBA or similar frameworks, and also works just as well when components are implemented directly as part of a BlockEntity, Entity, etc.
Registering Component Types
Creating a new component type is straightforward:
Note that two components can share the same type. This may be useful when the same component type could have a different purpose or meaning for the same provider.
The second parameter to
ComponentRegistry.createComponent()
is the value that should be returned when the component is absent, and made visible viaComponentType.absent()
. Avoid usingnull
here - cleaner code results when the absent value is a no-effect dummy instance.Using Components
Obtaining a component instance is a two-step process:
Call a variation of
ComponentType.getAccess()
to retrieve aComponentAccess
instance. Currently there are methods to get access from blocks (or block positions within a world), entities, and from Items, which may or may not be held by a player. Future versions may add access methods for other game objects if there are interesting use cases for them.Use a variant of
ComponentType.get()
to get the actual component instance, or the absent value if the component is unavailable.ComponentType.get()
accepts two parameters, both of which can be omitted:Direction
For components that are accessible via a specific side. Passnull
(or call aget()
variant without this argument) for components that have no side or to get the non-specific instance. Component types that do not have sides should ignore this parameter and access attempts from a specific side should always provide it, unless it is somehow known to be unnecessary.Identifier
Components may optionally have named instances and accept access requests for a specific named instance. This may be useful, for example, with machines that have more than one input/output buffer that may be accessible from the same side. Such a machine could return a different instance depending on which named buffer was requested. Use of this feature is optional and implementation-specific; This API currently makes no attempt to standardize these identifiers or their meanings.Often, access to a component is for a single use. In these cases,
ComponentType.acceptIfPresent()
can simplify code. It works likeget()
but also accepts aConsumer
for the component type and if it finds a non-absent component instance it applies the consumer and returns true. It returns false when the component is absent.Similar convenience is offered by
ComponentType.applyIfPresent()
. It accepts aFunction
over the component type, and returns the result of that function if a non-absent component instance is found, ornull
otherwise.Providing Components
The API can only return component instances that have been mapped via
ComponentType.registerProvider()
, like so:While access to components requires two steps, provisioning is handled by a single function. All of the information about the world/block/item/entity/side/id is marshaled into a
BlockComponentContext
,EntityComponentContext
orItemComponentContext
instance that the function consumes.Most block implementations will use a
BlockEntity
as their component holder/provider, but this is not required. The provider function must map the context data to a component instance. How that happens is unspecified. TheBlockEntity
value in the context can benull
, butWorld
andBlockPos
values will always be present.Item Actions
The API includes an opt-in system for adding behaviors to items that to perform some action when used on blocks that contain or are associated with a component.
ComponentType.registerAction
associates a potential action with a component type and one or more items. The first argument is aBiPredicate
that accepts anItemComponentContext
and a component instance. The intent is that multiple consumers can be registered for the same item/component, and processing will stop after any consumer returns true. Order of execution is unspecified, but this should not matter much in practice - the player can only be holding one item and clicking on one block at a time.Note these actions have no effect unless mods invoke the action handler in a block's
onUse
method (or wherever is appropriate) usingComponent.applyActions
orComponent.applyActionsWithHeld
. This should only be done server-side.