Skip to content
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

CraftingInventory is not a Grid #1710

Merged
merged 2 commits into from Dec 7, 2017
Merged

CraftingInventory is not a Grid #1710

merged 2 commits into from Dec 7, 2017

Conversation

Faithcaio
Copy link
Contributor

@Faithcaio Faithcaio commented Dec 1, 2017

SpongeAPI | SpongeCommon | SpongeForge

A crafting inventory is not a grid but contains one.

@Cybermaxke
Copy link
Contributor

Cybermaxke commented Dec 3, 2017

@Faithcaio Could you also fix the following issue in this PR, it's related to Container methods still using Causes: #1665

@Faithcaio
Copy link
Contributor Author

@Cybermaxke it seems they dont even have an impl yet.
Same for getViewers hasViewers canInteractWith

@DosMike
Copy link

DosMike commented Dec 4, 2017

The biggest problem with Inventories in my opinion is that there is no Slot Inventory.getNthSlot(int n).
The InventoryBuilder returns a SpongeCommon CustomIventory, that has getter and setter for Slots (getStackInSlot and setInventorySlotContents) , but those are not exposed to the API, so I'm using reflection and a signature scanner to use setInventorySlotContents for my menus. @codeHusky seems to be using the iterator and a counter variable when setting items at a specific slot
For me it is kinds important to be able to directly access slots in some way, and I think if makes writing menus with toggles (click at slot x, item at slot x e.g. changes color) way easier.

Thinking about it: would it be possible/viable to add something like Inventory.getSlot(int... position)? I'm thinking about non-grid-inventories being accessed as kind of grid inventories like this

public Inventory Inventory.getInventory(int... position) {
  Inventory target = this;
  int n=0;
  for (int index : position) {
    if (index < 0 || index >= target.capacity())
      throw IllegalArgumentException("Index "+n+" ("+index+)" was out of inventory bounds (max: "+target.capacity()+")");
    if (target.capacity()<=1) //Slot or Empty
      throw IllegalArgumentException("Position argument has more depth than this inventory");
    // go to 'index' sub-inventory:
    Iterator<Inventory> it = iterator(); for (int i=0; i<index; i++) target = it.next();
    n++;
  }
  return target;
}

For example in the MainPlayerInventory this would allow access to a slot in the GridInventory with gridInventory.getInventory(row, slot) as convenience method.

@Faithcaio
Copy link
Contributor Author

Faithcaio commented Dec 4, 2017

@Mumfrey ^?

Thats what query is for.
But #query(SlotIndex.of(index)) (SlotPos for x/y coordinate) returns ALL slots that are at that SlotIndex including those from rows, columns etc.

You can also query for OrderedInventory.class or GridInventory.class
and then after an instanceof check use the methods on there. #getSlot(index) and #getSlot(x, y)

Anyways this would have to be another PR.

@DosMike
Copy link

DosMike commented Dec 4, 2017

Oh, didn't know query accepts a SlotIndex instance as there's now overlaod with SlotIndex and the javadocs for query(Object...) don't mention it either as far as I can see 🤔

@Faithcaio
Copy link
Contributor Author

SlotIndex etc. are InventoryProperties
#query(InventoryProperty<?, ?>... props)

@Faithcaio Faithcaio merged commit e71c454 into bleeding Dec 7, 2017
@Faithcaio Faithcaio deleted the refactor/inventory branch December 7, 2017 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants