Can't integrate new storage bus inventory using just AE2 API

[jaquadro]

I'm trying to add a new external inventory compatible with the storage bus (Storage Drawers mod; like JABBA or other DSU blocks, but not DSU-compatible).

Registering a handler that returns a plain IMEInventory is sufficient to make interaction work, but not to keep the terminals up to date. This is documented in the IExternalStorageHandler API, and it specifies that an IMEMonitor must be implemented instead.

The built-in storage integration in the AE2 code base uses the non-API MEMonitorIInventory/IMEAdaptor combination to bridge this gap. I tried implementing a compatible IMEMonitor class and had no success. Registering with those internal classes via reflection resulted in successful interaction with my block.

On further inspection, the MEMonitorIInventory object is getting special treatment in the storage bus code. The storage bus will periodically poll an MEMonitorIInventory, but not other IMEMonitors (which have no polling method).

[thatsIch]

The general idea was, that if you implement that you generally want to introduce a new system to AE2 like EC2 is doing with fluids and thus have the full control over it, how to display and interact with it, using AE2 just as a host.

If you just want to introduce another inventory to it, the normal storage bus should suffice. Else can the MEMonitoryIInventory class be re-implement on your own if you implement your own StorageBus. Other than that, I can only ask for your source and how no success is shown in your case.

For further discussion the IRC might be a better place.

[jaquadro]

The normal storage bus won't suffice for nontraditional inventories because the terminal won't recognize more than 64 amounts per slot. Implementing an IMEInventory breaks past that barrier but the terminal won't update correctly without being wrapped by an MEMonitorIInventory.

I figured nontraditional inventory support would be one of the more common uses for the API, but it seems that's not supported and everyone who needed that support had the code directly integrated into AE2 along with their corresponding APIs (DSUs, Factorization, BetterStorage).

I see a few options: 1.) Introduce a new API entry to construct an opaque MEMonitorIInventory/IMEAdaptor pair with a given IMEInventory and BaseActionSource (e.g in StorageHelper: IMEMonitor createStorageBusMonitor (IMEInventory inventory, BaseActionSource src);) 2.) Introduce a new interface descending from IMEMonitor that adds a tick method, then change the storage bus to recognize this interface instead of MEMonitorIInventory (note I haven't fully examined the storage bus to see if other non-IMEMonitor methods are used). 3.) I turn over my integration code and corresponding APIs and it lives in AE2 like all the other third party storage integration.

What's your preference?

[jaquadro]

Github is eating my angle brackets so just pretend I referred to all the APIs correctly in the past couple posts.

[thatsIch]

There is a PR [1] on Forge for a convention for single stacks inventory with higher stack size than 64. Since it is still not pulled for like 4 month or more we have to pull these in for special integration and thus special treatment.

1. too many dependencies for an API imo
2. not sure about a ticking method on that, need to dig into that and check out if its a viable solution
3. probably prefered atm. Probably also least confusing for users.

---

[1] https://github.com/MinecraftForge/MinecraftForge/pull/1136

[yueh]

1.) Is probably the best way. We are already doing something similar with GridNode.

2.) Cannot really say anything about it currently. This might open up some internal behaviour. I would need some time to look into it.

3.) This might be an option, but I am not really convinced about it. Probably the same way the other integrations are bugging me. Personally I would probably prefer a multiproject setup with one "compat" project for external storages (or maybe even one per mod). But still build it into a single .jar. Would be easier to scrap a single one if needed.

[jaquadro]

@thatsIch where does (1) involve too many dependencies? What you return is an MEMonitorIInventory (today), but the consumer of the API only sees an IMEMonitor which is already part of the API and the storage bus gets the object it wants.

[thatsIch]

@jaquadro oh you meant it like that, thought you wanted to publish the whole class

@yueh easy to say, hard to mock out, its doable though and also preferred in the long run. I do something similar in IntelliE

[yueh]

I am currently considering something like the createGridNode/createGridConnection in IAppEngApi Just a factory method which returns the internal class wrapping the custom implementation passed to it.

@thatsIch long term goal. Also not sure about the way IntelliE handles it. I am really seeing it as a multiproject build with different repositories. If ForgeGradle even supports something like this.

[thatsIch]

I have it all in one project but its the same shit. Since FG is based on Gradle, it does support multi-projects builds. In #ForgeGradle people are asking that on regular basis.

Since NEI is still blocking my dev env, you can take that on if you want :) Be god with you and guide you to a brighter future!

[jaquadro]

I do multi-project/single-jar on my Garden Stuff mod and it works well enough. If you guys go down this route and figure out how to create a merged mcmod.info from gradle, I'd be really interested to see that.

@yueh Yeah that's the kind of thing I was originally hunting around for. Take an IMEInventory and BaseActionSource and return something I can register. From my perspective, I don't care what the object is as long as the API guarantees that it will get special attention from storage buses.

[thatsIch]

@jaquadro feel free to submit a PR and we can review it together.

[jaquadro]

I'll submit one later this evening. My intention is to add the following signature to the API in storage.IStorageHelper:

IMEMonitor{IAEItemStack} createStorageBusMonitor (IMEInventory{IAEItemStack} inventory, BaseActionSource src);

If either of you think a different name or location would be better, you've got the rest of the day to make recommendations before I hit you with actual code.

[yueh]

I do not see any problem with it. But there will probably be some larger changes on how we manage the API, so it might be put on hold for some time.

@thatsIch you might want to drop by in our irc channel for some more informations.

[jaquadro]

PRs for API and implementation are submitted, which GitHub helpfully linked above.

[AlgorithmX2]

Wouldn't it make more sense to just remove the wrapping from AE2's handlers and add the wrapping in the storage buffers for non-monitors rather then adding an API point jut to generate stuff that AE2 should be doing on its own?

[jaquadro]

It probably would, depending on the answer to two questions:

• When an IExternalStorageHandler registers an inventory, is the IMEMonitor vs IMEInventory distinction important anywhere outside of the storage bus? By moving the wrapper out of the handlers, only an IMEInventory is registered.
• Is there any danger/drawback to the storage bus taking any arbitrary IMEInventory and trying to wrap it in the special monitor? I expect IExternalStorage handlers aren't the only source of potential inventories or monitors. How are standard inventories like chests dealt with?

[AlgorithmX2]

1. The Storage Bus appears to really only work with monitors ( Network Monitors or The Storage Bus Monitor Adaptor ) so I think that wrapping generic IMEInventories with the monitors would be fine, anyone who wants to implement a monitor still could, as there is potential for optimization with monitors you lose with the generic Polling monitor.
2. I doubt it, almost all the storage bus handlers appear to be using the generic monitor wrapper right now as it is, I noticed one didn't but I bet 90% chance that its broken ( better storage crate handler )

I've not looked at the code much recently, but I think this approach makes more sense. even if there are a few rough edges to iron out.

[thatsIch]

So.. whats the consensus on that, right now?

[jaquadro]

If there was no movement, I was going to dig into this after getting my mod update out (probably this weekend?). @yueh didn't seem to be in a hurry to integrate API changes with restructuring the API project anyway.

[yueh]

Handling it internally is probably the better option. So this might be just some quickfix, which might later go away. Introducing something to the API, which is likely to get pulled again is not a good design decision.

[jaquadro]

An internal design fix doesn't necessitate pulling the API entry back out as long as it can still fulfill its promise. I agree though that if it doesn't need to be added in the first place, so much the better. The documentation still needs to be updated since IExternalStorageHandler will be giving contraindicated instructions.

I will close out the PRs after I go through and am satisfied there are no problems with an internal fix. Unless someone beats me to it.

[thatsIch]

After some time thinking I also prefer to keep it internal. I dont think we need to expose that through the API.