Assorted issues with ComputerCraft support
LemADEC opened this issue ยท 3 comments
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<>();
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.
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).
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 :)