Create StorageBlock registry #10254
Create StorageBlock registry #10254
Conversation
…handles arbitrary storage block types
There was a problem hiding this comment.
I've already seen enough so far. I don't like the current implementation, requiring something to have a tile entity probably isn't the way to go.
On top of that, putting faith in the custom storage block to implement update calls to warehouse logic is an absolute no-go for me.
This needs a big refactor to even become viable.
src/main/java/com/minecolonies/api/tileentities/storageblocks/IStorageBlockInterface.java
Outdated
Show resolved
Hide resolved
| * | ||
| * @param inWarehouse Whether it's in a warehouse | ||
| */ | ||
| void setInWarehouse(final boolean inWarehouse); |
There was a problem hiding this comment.
I feel like there should be a better way to do this, rather than leaking unnecessary methods for blocks stored in the warehouse.
There's a very specific implementation behind these methods that all storage blocks would have to follow, which makes me think this is an ill-suited method to exist here.
There was a problem hiding this comment.
Yeah. I think I'll actually move all of the logic to update the warehouse out of the racks. It seems like we should instead have an onChange event that the warehouse subscribes to for all of it's containers. And then the warehouse takes care of updating itself when it receives those update events. And then the owners of the storage blocks just need to worry about firing the onChange event when appropriate. WDYT?
There was a problem hiding this comment.
Not quite, the implementation that handles this is in AbstractTileEntityRack, and it basically causes an update to every deliverable item to update their respective requests.
In another comment I suggested writing this to an abstract class, and I'd put it in there with a final method that can't be overridden.
So you have a method setStackInSlot which is implemented to first update the data in the rack, return a boolean if successfull.
If successfull, trigger an update to the request system with the same input stack.
| * | ||
| * @return The current level | ||
| */ | ||
| int getUpgradeLevel(); |
| /** | ||
| * Upgrades the size of the storage, if applicable. | ||
| */ | ||
| void increaseUpgradeLevel(); |
There was a problem hiding this comment.
Hmm, I'm not convinced on this one. The upgrades will be very storage block dependent. For example, does an upgrade for Iron Chests mean we add new rows to the chests, like we currently do with Racks? Or do we not upgrade the chests at all and actually just move from Iron Chests -> Gold Chests -> Diamond Chests (and only allow them at specific building levels). For Storage Drawers, it probably makes the most sense to just base the number of upgrades a drawer can have based on the building level. For AE2, it'd probably something like the size of disks that are allowed or the number of connections to the AE network.
It seems like it will have to be defined by the owner of the addon.
There was a problem hiding this comment.
What I'd do instead (because this is actually needed on the block side to set an external level), is give this one a default implementation of nothing.
That way the majority of the blocks won't be forced to implement this with nothing inside of it, since it's not something that the majority of blocks will use.
Alternatively, define a separate interface IStorageBlockUpgradeable which contains this method, then the logic is opt-in.
src/main/java/com/minecolonies/api/tileentities/storageblocks/registry/StorageBlockEntry.java
Show resolved
Hide resolved
src/main/java/com/minecolonies/core/client/gui/WindowHutAllInventory.java
Outdated
Show resolved
Hide resolved
src/main/java/com/minecolonies/core/client/gui/WindowHutAllInventory.java
Outdated
Show resolved
Hide resolved
src/main/java/com/minecolonies/core/colony/buildings/AbstractBuildingContainer.java
Show resolved
Hide resolved
| * @param building The building that is being constructed. | ||
| * @return Whether the storageblock should be included in building containers automatically. | ||
| */ | ||
| boolean shouldAutomaticallyAdd(final IBuilding building); |
There was a problem hiding this comment.
Also this method feels a bit weird, this implies there's a manual way to add storage blocks.
I'd leave this to the match function inside the registry entry.
There was a problem hiding this comment.
My intention is to have manual ways to add storage blocks for my addon using a scepter
There was a problem hiding this comment.
Then I'd handle this inside your matches function for the storage block. Just return false for any block unless it's added in the allow list.
Additionally you can add containers after registration by accessing the container list in the building.
| @@ -116,32 +117,36 @@ private void locate(final Button button) | |||
| { | |||
| final int row = stackList.getListElementIndexByPane(button); | |||
| final ItemStorage storage = allItems.get(row); | |||
| final Set<BlockPos> containerList = new HashSet<>(building.getContainerList()); | |||
| containerList.add(building.getID()); | |||
| final Set<AbstractStorageBlockInterface> containerList = new HashSet<>(building.getContainerList()); | |||
There was a problem hiding this comment.
This is probably not going to be functional anymore.
The list is wrapped in a set, however the block does not have an equality check anymore.
I'd probably make the building its container list be a set itself and not a list. And implement a proper equality check in the AbstractStorageBlock,
Introduces a new Registry for StorageBlocks that allows addon authors to introduce new storage blocks besides Racks that are usable within the colony. This should be a no-op for vanilla Minecolonies.
With these new registry entries, you define an interface for how the colony interacts with a particular storage block, how storage blocks are mapped to the particular interface, and whether these storage blocks should be added automatically to buildings when in the schematic.
Testing
Review please