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

Invpipes #1403

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Invpipes #1403

wants to merge 12 commits into from

Conversation

warjort
Copy link
Contributor

@warjort warjort commented Jan 18, 2021

What:
This is not really a PR, more an RFC. I will try to convert it to a draft PR if I can figure out how do it.

This fixes the inventory pipes so they useable.

TLDR: You can connect inventories together using inventory pipes then use them as one big item source/sink.

I have got the thing working, but I haven't done any real testing in anger.

How solved:
There are few main parts to what I have done:

  1. A new item called an inventory pipe. This allows you to create an inventory pipe network.
    The inventory pipe has all the usual features like painting, wrenching, covering, etc.
    The inventory pipe network already exists in the codebase but it has some bugs (I have fixed the ones I found) but
    then it is still unusable as nothing uses the network.

So I have introduced a way to add a UI to any pipe when right clicked (by default it does nothing).
For the inventory pipe this shows the consolidated inventory which you can manipulate.
This reuses the widget created for MetaTileEntityWorkbench.

  1. I have introduced a new capability called IStorageNetwork. This lets you manipulate the consolidated inventory.
    I have made the inventory pipes implement this capability.

  2. I have introduced a new cover called the CoverStorageNetworkInterface.
    The cover can be placed on anything implements the IStorageNetwork capability, i.e. it goes on the pipe.
    This works like a Conveyor (the code is basically a copy) and has all the features of one (including all the item filters), except that it moves between an IStorageNetwork and a normal inventory.
    The idea is that you put one in export mode on a machine to move items from anywhere in consolidated inventory then one in import mode to return the outputs.

That's it. This is a basic proof of concept so I can ask for feedback.

Outcome:
N/A

Additional info:
I have spread a number of "Review:" comments in the patch which I will try to summarise here:

  1. The main review is on the gregtech.common.covers.CoverStorageNetworkInterface which is a bit long to reproduce here.
    It outlines possible future changes to make the inventory pipes more useable.

  2. How do unique numbers get assigned, e.g. for new metaitems and covers?
    I recommend adding something to the README in the project to outline this policy.

  3. Is it ok to change the IPipeTile in the way I have introduced the UI handling?

  4. The new capability IStorageNetwork is basically a simpler version of IItemList which was a gregtech.common interface before. Is the way I did this (including the IItemInfo) ok?

  5. For the rendering, I didn't use the existing renderer since I couldn't figure out what model I needed to create (it missing in the codebase).
    Instead I created a pipe renderer based off the fluid pipes, except they are coloured Aluminium.
    The new cover reuses the conveyor assets. I didn't want this PR rejected because of my lack of artistic skills.

  6. The cover is only present in LV tier, the other tiers will need creating once it becomes fixed so future changes don't have
    be copied multiple times.

  7. Finally there is a big hack to stop an NPE when loading the inventory pipes. see WorldInventoryPipeNet.
    To fix this properly will need a change to how pipe nets are deserialized.
    I didn't want to do that for this prototype since it means messing with the code for cable and fluid pipes which are currently unchanged by this PR.

@warjort
Copy link
Contributor Author

warjort commented Jan 18, 2021

The "convert to draft" option doesn't work for me?

@Archengius
Copy link
Member

Regarding idea:
Initially these pipes were meant to be connectable to the next tier of worktable and serve as an early game storage network alternative.
I definitely like what you did there, but primary point of them is providing additional inventory access to worktables, and in my opinion, it should be implemented too from the start to make these pipes usable.
Other thing to consider: these pipes should probably require single-block controller/charger to be able to move items through the network, otherwise it feels like a very cheap way of connecting inventories into networks.

Regarding implementation: I'm currently a bit busy and cannot really review it properly, but i'm sure LAGIdiot can do it on his own.

@warjort
Copy link
Contributor Author

warjort commented Jan 20, 2021

I have added a simple prototype for the energy.
Every inventory pipenet has a buffer which can be charged by connecting power to any inventory pipe, even solar panels work.
Performing any operation takes 1 EU - hard coded like everything else ATM.

The energy works like the fluids in fluids pipes. A downside to this approach is that energy is lost when two networks merge and there is not enough capacity to store all the EU. On a split the energy is just divided up between the two new networks.
This energy loss could be solved by having the capacity based on number of pipes like the fluid pipes?

This initial commit is intended so you have something to play with and discuss how energy actually works.
e.g. is it per operation, per item, an ongoing charge per connection or network?
What voltages, amperages and capacities should this have?

@warjort
Copy link
Contributor Author

warjort commented Jan 20, 2021

I tried doing the idea of using of an external charger/controller.

But this runs into complications where during chunk loading, the pipes are loaded before the charger(s), which is pretty much always, because you will have more pipes than chargers.

I am not saying it is not doable, but it would need some kind of lazy energy container like what is done for the item handlers of connected inventories.
i.e. During loading, it would have to put in a place a dummy energy container which would get replaced by the charger's real container when it is available, and revert back to the dummy if the charger gets unloaded.

I have a kind of working implementation without the reloading stuff.
But it is a lot more complicated than what I just committed.

@LAGIdiot
Copy link
Member

Honestly when I opened this PR I was blown away by size of it. And it may take me a while to go through all of this.

First of all I have to acknowledge your bravery to created new system from nearly nothing (I would probably start from getting ideas in issue - but that is me).

Regarding your idea I like it. And I can see that you put quite lot of thought to it and even marked parts which need to be expanded. System of pipes which will be able to connect our machines (+ worktable, others) either directly or through cover seems really good. I think your initial idea could be expanded quite a bit and become indispensable part of GTCE. As already Archengius mentioned this system should not work for free and I would like to add it should be scalable (throughput based on pipe size as we have for fluids) also we should have covers for all tiers (but I guess we can generate these later when we have base established and polished).

I will need to do deep dive in this to give you more detailed and meaningful comments. (I will try to get to it as soon I get my dev environment working again).

@LAGIdiot LAGIdiot added status: help needed Extra attention is needed type: feature New feature or request open for discussion rsr: minor Release size requirements: Minor and removed status: help needed Extra attention is needed labels Jan 20, 2021
@warjort
Copy link
Contributor Author

warjort commented Feb 8, 2021

I have added a hacky prototype of a "keep in stock" cover.

TLDR: You add the cover to an item pipe attached to a machine and configure what items you want to keep in stock using the machine.
It will automagically find a recipe using the machine's recipe map and ingredients available in the inventory pipe's consolidated inventory.
It also works for multiblocks by having the pipe next to one of its input buses. Provided the multiblock is formed, it will use the controller's recipe map if it has one.

The implementation details are not what I want to discuss, I tried to do it without modifying any other code so it is full of hacks.
Instead the following are the real issues:

  1. Tiering: Currently to be consistent with other covers you are restricted to 8 x tier items in your recipe. That's items, NOT ingredients. I also added a restriction of a maximum keep in stock of 8 x tier for any item type.
    But I think the real tier restriction should be that an LV keep in stock cover can only do LV recipes (this is not currently implemented).

  2. Outputs: This cover will automatically remove machine outputs. This is to help with the below point on machine activity. Removing outputs counts towards the 8 x tier items per second.
    Removing outputs is not implemented for multiblocks.

  3. Machine activity: The cover will only try to do a keep in stock request when the following conditions are matched:

  • The machine is not active
  • There are no items in the input inventory (except recipe selectors like molds)
  • There are no items in the output inventory (so it can do the keep in stock calculation properly using items from previous requests)
  1. Recipe finding: The current implement is very brute force and ignorance. One major improvement would be having a reverse index of output -> recipe(s) in the recipemaps.
    Such a method would also be useful for CraftTweaker from my experience.

  2. Fluids: I haven't done anything with this. The cover just assumes you know what you are doing with recipes that involve fluids.

  3. UI: Currently the UI is just a repurposed item filter. It should really be a list with elements like;
    The requested keep stock item/amount,
    an optional section to specify the exact recipe you want (selectable from JEI),
    an information section containing something like the following:

  • Fully in stock
  • Processing
  • Machine is active (with a different recipe)
  • No recipe
  • Not enough ingredients
  • Machine is wrong tier for the recipe
  • Wrong recipe selector
  • Machine is not accepting inputs
  • No inventory space for outputs
  • etc.

@warjort
Copy link
Contributor Author

warjort commented Feb 18, 2021

but primary point of them is providing additional inventory access to worktables, and in my opinion, it should be implemented too from the start to make these pipes usable.

I have added integration for the inventory pipe network into the crafting station (actually its recipe resolver).

I did it by creating a StorageNetworkItemSource, which is kind of meta :-)
This works like the existing inventory/tile source except it uses an IStorageNetwork to get the inventory contents.

What this does mean is that if you have 10 crafting tables on an inventory pipe network, any updates to a chest on the pipe network will get processed 11 times;

  • once for the pipe network itself
  • once for each crafting station to "merge" the pipe network inventory with the crafting station's inventory

These updates are actually consolidated and done once every 20 ticks unless the user is actively crafting.

@warjort
Copy link
Contributor Author

warjort commented Apr 6, 2021

FYI: I have released a modified version of these changes as an addon project:
https://www.curseforge.com/minecraft/mc-mods/gtce-inventory
https://github.com/warjort/GTCE-Inventory/blob/main/README.md

This will let people play with it and give feedback, instead of it just sitting in limbo.

I still plan to integrate these changes into GTCE proper, but I will instead with your agreement do this in a staged manner in separate PRs.

The proposed roadmap is;

NOTE: In the addon project I have removed the ability to right click an inventory pipe and see the full storage network.
This functionality is instead available via the crafting station's additional tab.

@LAGIdiot
Copy link
Member

I see and I totally understand your action. I am still interested in getting this merged in GTCE but I am barely able to keep up with small PRs to just fill releases. But I am doing some changes on Discord to get more people do reviews. So I expected that I will start having more time in not so far feature to get deep look at this or someone else will join me on it.

@Irgendwer01
Copy link

@warjort there should be some kind of Conversion of Blocks when migrating from the Addon that Players don't lose their System when migrating from Addon

@warjort
Copy link
Contributor Author

warjort commented May 27, 2021

@warjort there should be some kind of Conversion of Blocks when migrating from the Addon that Players don't lose their System when migrating from Addon

I don't think that is necessary, the 2 systems should be usable in parallel.

The main stumbling block for migration is that it is not guaranteed that the blocks/items accepted into GTCE will be 1 to 1 with those that exist in GTCE-inventory.
When the code is in GTCE it would be a good idea to add conversion recipes to gtce-inventory for items that are the same or similar.

But having the two pipe systems join together into a single pipe network seems unlikely because of the way pipe networks are implemented (e.g. they will be stored in different files in the save data).
There are also issues like I think @Archengius wants the inventory pipes to have external chargers for the energy rather than storing it internally.

@Irgendwer01
Copy link

@warjort Another Question: Why this thing before Item Pipes?

@warjort
Copy link
Contributor Author

warjort commented Jun 5, 2021

Why this thing before Item Pipes?

I don't understand your question.

@Irgendwer01
Copy link

Why this thing before Item Pipes?

I don't understand your question.

@warjort I mean, why do u want integrating Invpipes before Item Pipes got integrated into GTCE?

@warjort
Copy link
Contributor Author

warjort commented Jun 5, 2021

Invpipes are item pipes. Invpipes is short for inventory pipes.

@Irgendwer01
Copy link

Invpipes are item pipes. Invpipes is short for inventory pipes.

Hmm, ok, for me it looked more like Logistics Pipes or Applied Energistics 2 like system because of autocrafting and all that stuff

@warjort
Copy link
Contributor Author

warjort commented Jun 5, 2021

Hmm, ok, for me it looked more like Logistics Pipes or Applied Energistics 2 like system because of autocrafting and all that stuff

That is built on top of it.

At its heart, the feature is a consolidated inventory exposed as a capability.

Tictim referenced this pull request in Tictim/GregTech Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open for discussion rsr: minor Release size requirements: Minor type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants