-
-
Notifications
You must be signed in to change notification settings - Fork 344
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
Queryable Unified Inventory API #443
Conversation
Can I have a cookie for being patient and waiting for this PR to be made? |
@gabizou no you may not. |
* TODO Flesh out javadoc from proposal document. For now, see proposal doc | ||
* here: https://github.com/SpongePowered/SpongeAPI/pull/443 | ||
*/ | ||
public interface Inventory extends Iterable<Inventory>, Nameable { |
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.
Do all inventories really have Translatable names, even plugin-created ones?
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 considered putting this on Container
instead but in vanilla minecraft every IInventory
is IWorldNameable
and thus anything which is an Inventory basically has a name. I extracted that out to Nameable
partly because it's a useful interface to have on its own merits but also because we could potentially graft the Nameable
onto the hierarchy somewhere else if we don't like it being on the base interface.
Food for thought at any rate. Where I'm not sure about stufff I've erred on the side of simplicity and consistency with the view that we can throw it all in the bin if we feel the inclination.
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.
Wouldn't the name of the inventory be a Translatable?
Some small notes to the code style:
I've also noticed a few events in here that are already in the SpongeAPI. |
/** | ||
* Raised when an entity equips an item. | ||
*/ | ||
public interface EntityEquipEvent extends EntityEvent, InventoryEvent { |
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 this the same as EntityEquipmentChangeEvent
?
Okay that's fair enough, this is my first contribution to the API so I expected their would be style differences. I always use the modifiers just because it seems more readable but I can remove them no problem.
To be honest the plan was to flesh out the javadoc later, I wanted to wait until all the I's were dotted and T's were crossed but I've been too busy and @Zidane was probably going to murder me if I took any longer. Basically take a view that if there is some Javadoc, it'll get fixed later when I have two seconds to rub together, either that or someone else can beat me to it.
This isn't anywhere in the style guide, I'm not sure how I was supposed to know this.
Likely because @gratimax already added them before they were added to the API. I'm sure redundant events can be removed. Any ideas which ones? |
@Mumfrey Wasn't sure if the PR was already ready to be reviewed on code style, so I just thought I do that so we can fix them. :)
Some more things are mentioned in our contribution guidelines although it's not complete. I guess you were not able to know this, but now you do. ;)
I've commented on the (possibly) duplicate events above. |
Personally I tend to focus on the important aspects of code style (readability, wrapping, stuff having some kind of javadoc rather than none) and deal with the more fiddly parts in a second pass, at this point as long as it builds then people can review the structure of the PR and other issues can be mopped up going forward.
I was including the contribution guidelines in my above assertion, there is no mention of full stops other than specifically mentioning they should be omitted on at clauses.
I'll leave that one for @gratimax to take a view on. I've removed the |
First off, glorious pull request. How, in this new system, would you go about taking X amount of items from a given inventory? I imagine you would you create a query for the ItemType, but then what? Iterate over that inventory, removing item stacks until you've removed enough? |
Yes, because remember that your result set is an You need not iterate over the inventory because you can consume items directly from the query result (since it's an Inventory arrows = player.getInventory().query(ItemTypes.ARROW);
if (arrows.isEmpty()) {
// boo, no arrows
return;
}
ItemStack quiver = arrows.peek().get(); // We know it's present so calling get() is safe
quiver.setQuantity(quiver.getQuantity() - 1); // Decrement the stack quantity However, I think a nicer way (which I will add now you mention it) would be: Inventory arrows = player.getInventory().query(ItemTypes.ARROW);
if (arrows.isEmpty()) {
// boo, no arrows
return;
}
ItemStack arrow = arrows.poll(1).get(); // Consume a stack of 1 array from the result or even: Optional<ItemStack> arrow = player.getInventory().query(ItemTypes.ARROW).poll(1);
if (arrow.isPresent()) {
// blah
} EDIT - there we go, see commit fe8375e |
This is awesome in all of its definitions, what led you to come up with a system so different? |
fe8375e
to
d7a8842
Compare
@modwizcode there's honestly nothing really outlandish in this proposal, it's more the fact that everyone is so entrenched in the existing Inventory architecture (which, let's face it, is bloody awful) that nobody has really taken a step back and said "wait a minute, how should this work?" So much interaction with inventories to date has required far too much prior knowledge of the internals of the inventory in question, I simply tried to come up with a more expressive way of interacting with inventories. |
Quick comment because I'm on my phone and can't really look through the code: instead of |
@kitskub they're just the interfaces, how they're implemented is up to the implementation. |
@Mumfrey okay. Sorry, like I said: can't read the code at the moment. |
Well done. 2 things:
And a simple question: My Solution: Then Then Then If it was rejected. revert everything so that no items are lost (and drop the book). Does it work like this? |
Hmm. Interesting. What's the use-case just for my curiosity?
Okay.
ItemStack rulebook = ...;
inventory.query(Hotbar.class).first().set(rulebook); for other slots: Inventory result = inventory.query(Hotbar.class);
if (!result.isEmpty()) { // check the query actually succeeded
Hotbar hotbar = result.<Hotbar>first();
hotbar.set(3, rulebook); // Hotbar is an OrderedInventory so we can use positional set
} or using a fully query-based version: inventory.query(Hotbar.class).query(new SlotIndex(3)).set(rulebook); |
First of all: Awesome API! (And nice and complete description with descriptive images) @Mumfrey I guess @boformer tried to add an I have three question myself.
|
@ST-DDT I assume it would be like this: inventory.query(Hotbar.class).query(new SlotIndex(/* selected slot here */)).set(rulebook); |
* The maximum number of stacks the Inventory can hold. Always 1 for | ||
* {@link Slot}s and always 0 for {@link EmptyInventory}s. | ||
*/ | ||
int capacity(); |
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.
A method for summing up all ItemStack
sizes would be nice.
Inventory.query(ItemTypes.ARROW).totalCount();
Returns the total number of arrows in the inventory.
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.
Already suggested below.
@kenzierocks IMO this will replace the item at the given slot, removing it from the inventory, instead of moving it to another slot. (This is what i expect the method to do) |
* objects in the inventory as required to accomodate the entire stack. The | ||
* entire stack is always consumed. | ||
*/ | ||
void set(ItemStack stack); |
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.
Maybe this should return Optional<ItemStack>
for cases where an other Item has been thrown out of the inventory in order to add said item. (Map.put()
also returns any previous entry)
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 better handled by an event imo.
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.
has been thrown out of the inventory
I meant the old item in the slot that has been replaced by the new item.
Using events to get the replaced item would totally break the call chain here and will most likely force me to never run this method in a situation where it replaces another item.
76f8cb9
to
4506d3e
Compare
Is this PR mergeable? |
So how would you backup an entirw inventory with this system? Oh, and could you make inventories serializable by default? |
- Implement EmptyInventory in the API. - Default some Inventory methods to make implementation easier. Signed-off-by: Chris Sanders <zidane@outlook.com>
…@wysohn getting it to work. Please see the //TODO comments in InventoryEditManager for more details about what is broken.
This pull request encapsulates a proposal for an Inventory API for Sponge. At the core it is based upon #242 by @gratimax but is revised and expanded to include the proposition outlined below. Please read all of this document before commenting on design specifics within this PR.
This is my first PR to the API side of things so be gentle.
Design Proposal for a Queryable Unified Inventory API
Many of the proposed systems for dealing with inventories in Sponge have been very close to the underlying Minecraft implementation of inventories, there are a number of drawbacks with this model:
Inventory topology is bifurcated into "Inventories", generally describing an actual container (the Model), and "Containers" which present a code-facing view of one or more "Inventories" (the ViewModel). Which has its own problems:
The View in this instance (taking MVVM as our template) is a client-only user-facing object responsible for displaying the Container to the user.
Working with different types of inventory requires prior knowledge of the inventory's internal layout (interface is coupled invisibly to implementation) because there is no way to obtain meta information about a specific Inventory or Container. This means that:
Because of the blurring of the distinction between Inventory and Container, functionality is cross-contaminated between the two concepts, and this lack of separation of concerns tends to (in turn) pollute any API which tries to faithfully replicate it.
Design Goals
The aim of this proposal is to address these shortcomings in the following ways:
Starting Assumptions
As Morpheus famously said to Neo: "free your mind".
To begin talking about a new structure for Inventory interaction, it's necessary to forget everything you currently understand about Inventories. Done that? Okay, let's start with the basic concepts.
Common Inventory Operations
If we make no other assumptions about an inventory, we can essentially begin by declaring that, in many ways an Inventory exhibits the same behaviour as a
Queue
in that we can add items to the Inventory, consume items from the Inventory, and get the underlying capacity of the Inventory.Note that our base implementations of
poll
andoffer
don't provide any position information, this is because we are not making any other assumptions about the structure of the Inventory at this point. While this might seem like an odd baseline, it begins to make sense when we consider that with a query-based Inventory API, we will be returning inventories which fall into one of the following three categories:Inventory
with a single stack is effectively aSlot
. By keeping theInventory
interface simple enough that aSlot
can reasonably extend it, we have simple unified way of a query returning anything from an entire Inventory, to a specific set of matching slots, to a single slot containing an Item we're looking for.Basis and Purpose of Queries
Before we look at how queries can work, let's first take a look at a simple use-case to understand the purpose of queries in the first place.
The
PlayerInventory
class actually encompasses several groups of inventory slots we may be interested in. These groups are internally (physically) separated into two arrays: the Main Inventory and the Armour Inventory. The Main Inventory comprises both the hotbar area (the 9 inventory slots always displayed on the screen) and the rest of the player's main inventory. Representing this as a basic Venn Diagram we see:When working directly with Minecraft, the two inner inventory arrays are available directly as public members of the
InventoryPlayer
class. However some additional functionality is available which supports both the ViewModel-esque contract required by Containers and also some additional convenience functions for working with only the hotbar slots. Some notable drawbacks with this mixing of concerns is that the following become apparent when working with thePlayerInventory
:InventoryPlayer
class)It should be noted that this scenario is far from unique amongst inventories, and only gets worse when dealing with Containers. For example let's take a look at a container which you're likely to be very familiar with,
ContainerPlayer
:This container presents a View of 3 separate Inventories (essentially ViewModels) which in turn represent 4 distinct underlying arrays of slots (essentially the underlying Models).
You may find yourself asking "so what the hell does all this have to do with queries?" and that's a fair question. Ultimately we have 3 ways of dealing with this mess in order to improve it from the point of view of API consumers:
instanceof
-and-cast operations which would have been required before.Presented with the options it's clear that option 3, using queries has merit.
Goals for a Query-based implementation
Fundamental to the concept of querying Inventories is the basic premise that:
In the
InventoryPlayer
example above, the relationship of the Inventory to its notional sub-inventories is as follows:The main idea of queries is that given an unknown
Inventory
instance, it should be possible to query for any sub-inventory or combination of matching sub-inventories, with the query returning either all sub-inventories which match the query, or an empty set if no sub-inventories matched the query.It's useful to bear in mind that our above definition of
Inventory
essentially means that everything can be represented as anInventory
right down toSlot
(defined as anInventory
with only a single position) and thus a more helpful representation when considering what can be returned by a query is the following:We're now in a position to specify what a query should actually return:
Which produces as a consequence some assumptions for
Inventory
itself:Inventory
Inventory
isIterable<Inventory>
and the returned iterator traverses the child nodes of the Inventory's hierarchyInventory
's leaf nodes should also be traversable via a method which returns anIterator
To give some examples, based on the hierarchy above:
Hotbar.class
should return the Hotbar inventoryItemTypes.TNT
should return an Inventory with all of the slots containing TNTInventoryRow.class
should return each row in the Main Inventory and the HotbarWhich produces the following assumptions:
Inventory
matches the query directly, it should return itselfInventory
should perform a depth-first search of its hierarchy. A matching child node will remove its parent from the results set if present.EmptyInventory
.We now have enough information to begin formulating our query interface.
Inventory methods for query results
Since the result of a Query will always be an
Inventory
, we add some decoration and methods to ourInventory
interface. Firstly, we have theInventory
extendIterable<Inventory>
as planned. Next we add a method for iterating the leaf nodes of ourInventory
and a convenience function for checking whether theInventory
has no slots:Depending on requirement, we may also wish to add some convenience methods to work with
Inventory
s which are result sets, for exampleInventory methods for executing queries
It is anticipated that the full scope and possibilities of queries will only become apparent in time, and also that the initial system should be sufficiently extensible to support quite a wide scope of potential queries. However the basic query types which should be in the initial release are:
Query by type
Query for sub-inventories matching the specified interfaces/concrete classes (effictively an
instanceof
check). Multiple classes can be specified and logicalOR
will be applied.Example 1:
Example 2: Logical AND (querying the result of a query)
Query by contents
Query for slots containing
ItemStack
s with items of the specified type. A logicalOR
applied between query operands.Example:
Query by property
As well as defining a (possibly multi-dimensional) hierarchy of sub-inventories, it is anticipated that some sub-inventory types will tag their members with additional data which can be used to include them in queries. For example, let's assume that we have a
GridInventory
interface:Assuming that
InventoryPos
extends some ficticious base classInventoryProperty
, we now have a mechanism for querying by any arbitrary properties we care to define:Query for sub-inventories where the specified property is set and
.equals()
the supplied properties. LogicalOR
is applied between operands.Example:
Query by name
Since in the real world,
Inventory
classes are always nameable, we can use aString
overload to query by name:Query by arbitrary operands
To promote extensibilty, even where completely unknown Inventories are in play, we should also include a general query interface which will allow arbitrary queries to be executed:
Query for sub-inventories using an arbitrary check defined by the inventory class in question.
Example:
Multi-Dimensional Query Model
So far, for simplicity we have outlined a straightforward hierarchical model of the inventory. However it is not the intention that underlying implementations should adopt such a simple model. To facilitate querying by dimensions, we can represent underlying sub-inventories in as many different dimensions as we like, and allow queries to return corresponding views. For example we can expand our simple example above to allow the Inventory to be queried by column:
In this example the
InventoryGrid
contains both row and column sub-Inventories. This is by no means the limit of what can be represented by further dimensions. However note that one tree should always be considered the master view (a notional appellation), a traversal of which visits each leaf node in a deterministic order which is the iteration order as experienced byslots()
.Essentially every Inventory should have a deterministic spanning tree, whose depth-first traversal will be the primary iteration order of the leaf nodes. This order is left to implementors to decide upon. Here is an example for the inventory structure shown above.
Example Spanning Tree for the Player Inventory
![](https://camo.githubusercontent.com/ba068bce38521dd3ed2db3b25fba3120fe93e9317a91545d9dbda1569b9149fd/687474703a2f2f6571322e636f2e756b2f70722f696e762f696e766869657261726368797370616e6e696e672e706e67)
Example traversal of the Spanning Tree
![](https://camo.githubusercontent.com/748cc73928f5c8363d8802aa6b01455309ea251dabe94c2352e1a2ff7a6dc5e6/687474703a2f2f6571322e636f2e756b2f70722f696e762f696e7668696572617263687974726176657273652e706e67)
Implications for Intended Usage
So far we have seen how queries can work in general, the main goals of incorporating queries into the API are to change the general way that consumers interact with Inventories. Without queries, a typical interaction with an Inventory might take place as follows:
PlayerInventory
is obtained from aPlayer
because it is advertised by the object.Now if the same code wishes to interact with a different type of Inventory, the code has to be adapted or rewritten, even if the other inventory has substantially the same behaviour or characteristics.
With a query-based approach, the interaction takes place as follows:
Inventory
.InventoryRow
sIf the same code wishes to interact with a different type of Inventory, it can do so as long as the Inventory has
InventoryRow
children.Taking the intended usage into account, we can deduce some additional characteristics our API should have:
Any API object which has an Inventory should simply return
Inventory
, with no requirement to return a particular subinterfaceConsumers should always obtain
Inventory
subclasses by querying for them (either directly or viainstanceof
check)Specific Inventory subinterfaces should still be provided within the API in order to satisfy queries, however they should not generally be returned and different implementations are therefore at liberty to select whichever interfaces they wish to implement - in other words there is no hard rule that says a
PlayerInventory
must haveInventoryRow
s as children for example, the query model means that this is purely optional and the only negative effect would be that consumers expecting such a structure would not be able to perform their operations.This works in both directions however, and while it is expected that all implementations will likely follow similar conventions (in order that queries can be relied upon across platforms), it's also possible that some implementations will be able to innovate, and extend their capabilities without in any way breaking backward compatibility.
The implication of all this is that the shape and design of the underlying Inventories is thus free to follow a logical representation of the underlying game without at any point breaking backward compatibility.
This in turn leads us to the conclusion that the number of "specific" inventories should be kept to a minimum, and general-purpose Inventory interfaces should be used wherever possible. The general-purpose Inventory interfaces should essentially represent characteristics of inventories, with little regard for specific Inventory types.
Relationship with Containers
It should be clear by now that a query-based model also solves the Container problem. By acknowledging that the line between Inventory and Container can often be blurry, we can treat Containers as Inventories for the most part (since we have essentially turned Inventory into a ViewModel at this point anyway, and all a Container really is is a ViewModel) and simply have them exist as yet another type of SubInventory interface.
Features not included within this PR
Taking a longer view, the following features are worthy of consideration