austinv11 / PeripheralsPlusPlus

A pseudo-port of miscperipherals, with other stuff as well!
GNU General Public License v2.0
25 stars 27 forks source link

Pocket Peripheral API Feedback #97

Open SquidDev opened 9 years ago

SquidDev commented 9 years ago

As requested from SquidDev-CC/CC-Tweaks#56 I'm going to give some feedback on the Pocket Peripheral API.

IPocketAccess

This isn't a requirement, but would be a bonus. Instead of passing around Entity and ItemStack instances everywhere it would be nice to have everything wrapped in an interface. It would look something like:

interface IPocketAccess {
  Entity getEntity(); // Gets the holding entity

  // Gets/sets the modem light on the pocket computer
  boolean getModemLight();
  void setModemLight(boolean value);

  // Same as `ITurtleAccess`. However this is specific for this upgrade.
  NBTTagCompound getUpgradeNBTData();
  void updateUpgradeNBTData();
}

This abstraction from the ItemStack means you don't have to manually set the modem NBT tag, or fiddle with other pocket internals. Personally I think that is cleaner, and means other people can implement tablets and implement this API without having to have identical internals.

It also means the peripheral container can create custom implementations of these and so each upgrade can have its own upgrade NBT tag.

Factories & #77

This is something I'd prefer to have, but adds another layer of complication, and is very different from how turtles implement it. So probably best to ignore.

Basically there would be two classes. IPocketUpgradeFactory. This implements information about the upgrade - adjectives, crafting item, id/string identifier and a method that creates the upgrade. IPocketUpgrade then provides methods that handle instance interaction - update, right click, and either implements or provides an instance of IPeripheral. This means you don't need to cast the IPeripheral instance in your update handler - because it is being called on the peripheral/upgrade itself instead.


What you have currently is very nice. I don't see any additional functionality/methods that need adding to it - though admittedly I'm not doing anything complicated with it. My only real complaint is having to fiddle directly with the ItemStack.

austinv11 commented 9 years ago

Makes sense, but I feel that the IPocketUpgradeFactory would be useless in this case. Because It may be a little awkward to deal with an instanced IPocketUpgrade which then itself provides another instance of an IPeripheral

But overall, I think I will implement pretty much everything you pointed out for the next major release of Peripherals++.

SquidDev commented 9 years ago

I agree, the only problem it gets around is the casting of peripherals, and adds far more complications than it is worth. You can tell I've been around Java too long when I start talking about factories. :smile:

On a totally unrelated point - why do peripheral containers store their peripherals in ComputerCraftHooks rather than in the peripheral instance?

Thanks! Now I've got to write some other pocket peripherals.

austinv11 commented 9 years ago

The peripheral container upgrade is a bit of a hack, the instances are stored where they are because they are accessed in a few places including ComputerCraftHooks

SquidDev commented 9 years ago

I implemented some of this for CCTweaks.

However I'm having issues with the NBT tag being saved (SquidDev-CC/CC-Tweaks#63) - changes to the NBT tag are not stored to disk. Any thoughts on why? - I can't seem to find a usage of you modifying the tags so I have no code to steal :frowning:.

austinv11 commented 9 years ago

I'll have to check that out.