PneumaticCraft: Repressurized

PneumaticCraft: Repressurized

50M Downloads

Assorted issues with ComputerCraft support

LemADEC opened this issue ยท 3 comments

commented

Minecraft Version

1.12.2

Forge Version

n/a

Mod Version

1.12.2-0.9.1-317

Describe your problem, including steps to reproduce it

Issue A: Pneumaticraft registers a provider for (nearly) every blocks in the mod. In practice, you only need one. Current approach is adding unnecessary CPU load.

Issue B: As per CC API documentation for IPeripheralProvider.getPeripheral(), the latest should return null when it's not a supported block. As it's currently implemented, all blocks implementing IPeripheral (from CC API), including those from other mods, will be rerouted by PneumaticCraft IPeripheralProvider.

Nota: the combination of both issues causes all blocks implementing IPeripheral to be called 45 extra times for each attach/detach events from ComputerCraft API while Computronics is installed.

Issue C: most IPeripheral methods, including attach() & detach() are called from the related computer thread.
Since multiple computers can attach to the same block, the related data structures should be thread safe.
Issue C': The notifyComputers() method is called from update() which is in the client/server thread. As such, it would also require thread protection when accessing the attachedComputers collections.

The following collection in PneumaticCraft isn't thread safe:

private final List<IComputerAccess> attachedComputers = new ArrayList<>();

private final List<IComputerAccess> attachedComputers = new ArrayList<>(); // keep track of the computers so we can raise a os.pullevent.

private final List<IComputerAccess> attachedComputers = new ArrayList<>();

Consider using a CopyOnWriteArray collection instead, it's naturally thread safe and well suited for this kind of usage (many read, few write, small size).

Any other comments?

There's probably a lag impact during chunk loading and unloading from those issues, I didn't profile to verify it's significant. For sure, fixing the above would prevent 'random' ConcurrentModificationExceptions.

commented

Update: after a little code perusal, B & C look pretty simple to fix.

However, for A, what do you mean by "only need one"? As far as I can see, registerPeripheralProvider() needs to be called for each block that can be a CC peripheral, which is indeed most PneumaticCraft blocks (providing info about pressure and/or temperature are methods common to nearly all machines in the mod).

Update 2: actually, I think I see what you mean. The getPeripheral() method gets the world/blockpos/face, so can get the tile entity there. And if that TE belongs to PNC and implements IPeripheral, return it, otherwise return null. I should be able to make me.desht.pneumaticcraft.common.thirdparty.computercraft.ComputerCraft implement IPeripheralProvider and just call registerPeripheralProvider(this) once in its preInit (and add a getPeripheral() method to that class).

commented

Thanks for the bug reports, there does appear to be some work to do here. I didn't actually write the CC integration code, and I'm not overly familiar with the CC API. Do you have a link to the docs you mentioned?

(Since you seem pretty familiar with how the code should work, I may have further questions for you. Also, pull requests are always welcome :)

commented

Fixed in 0.9.2 release