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

Refactor inventory #1585

Merged
merged 1 commit into from Dec 7, 2017
Merged

Refactor inventory #1585

merged 1 commit into from Dec 7, 2017

Conversation

Faithcaio
Copy link
Contributor

@Faithcaio Faithcaio commented Nov 5, 2017

SpongeAPI | SpongeCommon | SpongeForge

Restructure internal Inventory classes:

  • InventoryAdapter Adapter interface (implements Inventory)
    • MinecraftInventoryAdapter default implement most of Inventory
      • AbstractInventoryAdapter Adapter implementation (all types)
        • VanillaAdapter (IInventory)
          • Conceptual Inventory Adapters: OrderedInventoryAdapter etc.
        • SlotAdapter (Wrapper for inventories without real Slot)
          • specific SlotAdapters: FilteringSlotAdapter etc.
        • MinecraftQueryResultAdapter
        • IItemHandlerAdapter (Forge)
      • Adapter Mixins:
        • MixinTraitInventory Implement Inventory interface using Adapter Interface
        • MixinTraitAdapter Implement Adapter interface (based on Reusable/LensProvider)
        • TileEntity Mixins etc.
      • Forge Adapter Mixins: MixinInvWrapper, MixinItemStackHandler
      • Forge Wrapper: IItemHandlerAdapter
      • MixinSlot, (Container Slots)
      • MixinSlotItemHandler (Forge)
      • Sponge Inventories: MixinCustomInventory, MixinSpongeUserInventory
  • Lens
    • AbstractLens
      • RealLens Lenses for real inventories (gets real inventories if mixed in)
        • ContainerLens constructed with lenses of sub-inventories
          • ContainerPlayerInventoryLens
        • CustomLens (for Sponge Inventories)
        • Lenses for vanilla inventories: FurnaceInventoryLens etc.
      • ConceptualLens Lenses for inventory concepts
        • CompoundLens used for union operation
        • OrderedInventoryLensImpl etc.
      • DelegatingLens (used in Containers) spanning child is orderedlens ; child is delegate
      • DefaultIndexedLens fallback
      • QueryLens for query results
    • SlotLens -> SlotLensImpl, FakeSlotLensImpl etc.
    • DefaultEmptyLens
  • Fabric
    • MinecraftFabric (IInventory based)
      • CompoundFabric (based on 2 other MinecraftFabric)
      • ContainerFabric (based on Set of IInventory in Container)
      • IInventoryFabric (based on a single IInventory)
      • SlotFabric (based on a single Slot)
    • IItemHandlerFabric (based on Forge IItemHandler)

Move Adapter.Logic into its own class.
Move as much implementation into the same place instead of duplicating it.
Lazy Lens creation (MixinTraitAdapter) based on LensProvider interfaces
Reusable Lenses (lenses for TileEntities only need to be constructed once)

A nice sideeffect of this whole thing is that you can now query for TileEntity.class on Containers and you get it (for vanilla / TileEntities we applied mixin to).

Also Container structure is more uniform (Spanning Children are always VanillaAdapters with an OrderedInventory ; VanillaAdapter with delegating lens can forward to the real inventory + we can reuse the inventory lens in the container)

	ContainerChest 63 parent ContainerChest  sub: 2 
		VanillaAdapter 27 parent ContainerChest  sub: 1 
			OrderedInventoryAdapter 27 parent VanillaAdapter  sub: 27 |||||||||||||||||||||||||||
		VanillaAdapter 36 parent ContainerChest  sub: 1 
			OrderedInventoryAdapter 36 parent VanillaAdapter  sub: 36 ||||||||||||||||||||||||||||||||||||

	TileEntityChest 27 parent TileEntityChest  sub: 3 
		InventoryRowAdapter 9 parent TileEntityChest  sub: 9 |||||||||
		InventoryRowAdapter 9 parent TileEntityChest  sub: 9 |||||||||
		InventoryRowAdapter 9 parent TileEntityChest  sub: 9 |||||||||

	InventoryPlayer 41 parent InventoryPlayer  sub: 3 
		MainPlayerInventoryAdapter 36 parent InventoryPlayer  sub: 2 
			GridInventoryAdapter 27 parent MainPlayerInventoryAdapter  sub: 3 
				InventoryRowAdapter 9 parent GridInventoryAdapter  sub: 9 |||||||||
				InventoryRowAdapter 9 parent GridInventoryAdapter  sub: 9 |||||||||
				InventoryRowAdapter 9 parent GridInventoryAdapter  sub: 9 |||||||||
			HotbarAdapter 9 parent MainPlayerInventoryAdapter  sub: 9 |||||||||
		EquipmentInventoryAdapter 4 parent InventoryPlayer  sub: 4 ||||
		SlotAdapter 1 parent InventoryPlayer  sub: 0 
	ContainerFurnace 39 parent ContainerFurnace  sub: 2 
		VanillaAdapter 3 parent ContainerFurnace  sub: 1 
			OrderedInventoryAdapter 3 parent VanillaAdapter  sub: 3 |||
		VanillaAdapter 36 parent ContainerFurnace  sub: 1 
			OrderedInventoryAdapter 36 parent VanillaAdapter  sub: 36 ||||||||||||||||||||||||||||||||||||

	TileEntityFurnace 3 parent TileEntityFurnace  sub: 3 
		InputSlotAdapter 1 parent TileEntityFurnace  sub: 0 
		FuelSlotAdapter 1 parent TileEntityFurnace  sub: 0 
		OutputSlotAdapter 1 parent TileEntityFurnace  sub: 0 
	ContainerBrewingStand 41 parent ContainerBrewingStand  sub: 2 
		VanillaAdapter 5 parent ContainerBrewingStand  sub: 1 
			OrderedInventoryAdapter 5 parent VanillaAdapter  sub: 5 |||||
		VanillaAdapter 36 parent ContainerBrewingStand  sub: 1 
			OrderedInventoryAdapter 36 parent VanillaAdapter  sub: 36 ||||||||||||||||||||||||||||||||||||

	TileEntityBrewingStand 5 parent TileEntityBrewingStand  sub: 3 
		OrderedInventoryAdapter 3 parent TileEntityBrewingStand  sub: 3 |||
		InputSlotAdapter 1 parent TileEntityBrewingStand  sub: 0 
		FuelSlotAdapter 1 parent TileEntityBrewingStand  sub: 0 

@Faithcaio Faithcaio force-pushed the refactor/inventory branch 2 times, most recently from 1abba2a to 93c7543 Compare November 12, 2017 09:24
@Faithcaio Faithcaio force-pushed the refactor/inventory branch 3 times, most recently from c343a42 to bf2beff Compare November 25, 2017 10:37
@ryantheleach ryantheleach added status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc status: wip Work in progress labels Nov 26, 2017
@Faithcaio Faithcaio force-pushed the refactor/inventory branch 5 times, most recently from 1a47e05 to f2fce41 Compare December 1, 2017 21:54
@Faithcaio Faithcaio removed the status: wip Work in progress label Dec 1, 2017
@Faithcaio
Copy link
Contributor Author

I think I'm done with it for now.
Ready for testing.

@ryantheleach
Copy link
Contributor

@SimonFlash @codeHusky @TheoKah @ustc-zzzz @Zerthick @GreWeMa @DosMike

As developers with plugins that interact with inventories, I'd appreciate it if you guys managed to test these changes, and see if the inventories structure makes sense.

@Faithcaio Is a build able to be made for these guys to assist (if they wish to), or is there API changes that go along with this that they might need?

@Faithcaio
Copy link
Contributor Author

Faithcaio commented Dec 4, 2017

API is changed SpongePowered/SpongeAPI#1710 (but not much)

@ryantheleach
Copy link
Contributor

@Faithcaio I've edited it into the OP.

@SimonFlash
Copy link
Contributor

All of my inventory operations are functional on SpongeForge with 2555. The subset I use covers basic inventory operations, queries, and retrieving slots from indexes, so anything beyond that I can't confirm.

@Faithcaio Faithcaio force-pushed the refactor/inventory branch 2 times, most recently from 76fa6f9 to 7d55c45 Compare December 6, 2017 13:07
reduce code duplication
make some lenses reusable
make internal inventory structure more consistent
@Faithcaio Faithcaio merged commit ccbd879 into bleeding Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc system: inventory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants