Glowstone-Legacy
Glowstone-Legacy copied to clipboard
GlowstoneMC
Support for multi-block inventories (Double chests)
This PR adds preliminary support for Double Chest inventories.
The core of GlowDoubleChestInventory is GlowSuperInventory, which allows developers to create Inventory trees, that is, Inventories whose changes are delegated to other sub-inventories. A change in a child is immediately reflected in the GlowSuperInventory, and equally a change in GlowSuperInventory is also reflected in the original inventories.
Other cool stuff which this PR adds is:
- SuperSet: adds a set which delegates in other sets, and as in GlowSuperInventory, changes are propagated in either way. Employed to implement the viewers of GlowSuperInventory.
- SuperIterator: accepts a list of iterators, and it consumes them in order. Why? Well, you can then iterate through a list of inventories without building a temporary intermediate list, which is very efficient performance-wise.
- GlowInventorySlot: an inventory slot. This, and the new "slotIterator" method in GlowInventory allows for efficient linear navigation and modification.
Edited by turt2live
Related Links:
- Ticket: #263
- Old related PR: #351
@dequis I'm aware that there is another PR which also adds support for Double chests. In fact, we both talked yesterday in the IRC. However, we've taken different paths.
He has programmed an ad-hoc class for double chests. I intentionally wanted to avoid that, and therefore I created the SuperInventory, which is a generic class for handling multiple inventories as a single one. SuperInventory will come in handy if Mojang ever adds another "distributed" inventory.
When working with the SuperInventory, I also wanted to avoid copying items, so I'm working with references (ie references to other inventories) as much as possible, since it's the fastest possible way to interact with inventories. His way requires to copy the inventory each time an item is modified, which is really slow.
I'm missing however a few bits he has already working, like properly handling placement.
I can add the directional placing / connectioning of chests to your PR if you want me to.
Edit: Never mind, I've fixed it myself. I've submit a PR with the changes and severla bug fixes to your repo.
Note to self/other reviewers: This must be reviewed in conjunction with #351
Okay, this is pretty much finished. I'm only missing a few tests for new collection classes, but everything else is done.
As a note, this will be reviewed after #388 is pulled. Please include any other appropriate changes from #351 in this PR as #351 has been closed in favour of this PR. Thanks!
@socram8888 Your PR does not compile! Here's some information about the compile testing:
- Glowkit PR found: false
- Glowkit PR parsed: NONE
- Glowkit compiled: false
If your PR requires Glowkit, please ensure that an accurate link to the Glowkit PR appears in the description of your PR. If the information above is incorrect, please contact me on IRC (turt2live in #glowstone on esper.net) for more information.
If your PR does not require Glowkit, please fix your PR.
Thank you!
PR_BOT_CODE:3 (you can ignore this)
Fixed. Travis was complaining about a whitespace preceding a comment (?)
http://bamboo.gserv.me/browse/GLOW-SRV-PR358-71
@socram8888 Due to recent changes on master, this PR is no longer mergable. Please rebase as soon as possible!
Trivial conflict in the imports part of TEContainer
Merged branch here https://github.com/dequis/Glowstone/compare/pr-358
@socram8888 Please fix the file permissions in GlowInventory.java (change back to 644).
As for the rest of the PR, the organization of classes is not apparent (making it difficult to find things), but overall I do not see why such a complicated system is actually needed. When I first glanced at it almost a week ago it made sense, but now that I've scrolled through the entire diff it doesn't quite make sense.
I understand the motive to do so, but I fear as though it's too much over the top for right now. It looks good in terms of structure (besides a few quirks) however.
The reason this diff is so large is because of the SuperThing classes. Modifications to other classes are minimal.
I could have made an ad-hoc SuperInventory (actually, at some point that's how it worked: https://github.com/socram8888/Glowstone/blob/1face1034544c5a2a47729f9eaaf8ffee4e61bdd/src/main/java/net/glowstone/inventory/GlowBaseInventory.java) which could have employed other Inventories. However I think that those ad-hoc are in the long run always worst.
There is a reason Java collections are so widely used, and that's because of generics. Creating an ad-hoc class is a step backwards, even if it results in a less scary (ie smaller) diff.
Maybe I should move these to a library, much like Wolvereness Remappper, instead of compiling them as part of Glowstone?
I don't think moving them elsewhere is by any means useful. It's a few classes and would be an honest waste of time to set up the environment for using said library. As for the diff, this PR really does take two subject matters which are closely related and put them together. Generally this is considered a bad idea as now you have a single ticket for two issues. However, seeing as this is almost done, I'm fine with it as-is.
Please review the comments made previously when you get the chance.
Except for a few bits that by design can't be modified (namely using a null array instead of a populated Slot list, because everything has been redesigned to use the getSlot method and they expect a non-null value), PR should now be ready, and should also comply with the style guide.
Non-trivial merge conflict in GlowInventory - Normally I'd provide a merged branch but it's going to be better if @socram8888 takes care of it, I don't want to merge this without understanding it completely.
That merge conflict was caused because he has fixed a bug I also fixed on my own, which was basically that the "addItem" method didn't properly compare stacks (it only checked ID and damage, but not meta)
I was going to keep the "add(array)" working like that, to mantain plugin compatibility (since that's how CraftBukkit behaves), while adding a new method ("addItemStack(ItemStack, boolean)" - implemented but still not on Glowkit) which has an additional boolean to check or ignore meta.
Should I stash my changes or his?
I've ended up using my own fix, mostly because he forgot to also check the metadata on the remove method, which mine did.
There's a problem with which chest is considered the "top" chest in the double chest inventory. Below are some pretty pictures to help.
Legend
- Red wool: North
- White wool: South
- Pink wool: "top" chest
- Blue wool: "bottom" chest
- Blue/pink wool indicates where the second chest would be located
In vanilla,
As you can see, the east and south chests are considered to be the "bottom" chest
However, with this PR...
All chests on the logical left are considered the "top" chest.
EDIT
This PR actually produces the following results. Honourable mention to @dequis for getting in the way.
It is actually inverted from vanilla.