EngineHub / CommandBook

General and administrative commands
https://enginehub.org/commandbook/
GNU Lesser General Public License v3.0
145 stars 105 forks source link

Add API for Custom Kit Items #103

Closed NathanWolf closed 10 years ago

NathanWolf commented 10 years ago

This PR adds a simple API so that other Plugins may register an ItemProvider, which the Kit object will now use (via ItemUtil) when creating new items for a kit.

Each registered ItemProvider will be queried, and if it provides for the requested key, it can return an ItemStack to be added to the kit.

A use example for the Magic plugin (which I used to test this integration) is putting "wand: guest" in the "starter" kit. If Magic is present, it will provide a wand in the kit when requested.

This PR also slightly changes the way Kits work internally, the more invasive of the two changes. A Kit will now store an item id/amount pair, rather than a pre-generated ItemStack. This was mainly needed because Kits load before dependent plugins get a chance to register, but it also has the bonus side-effect of supporting randomized or otherwise dynamic items (e.g. "wand: random(20)" in the Magic plugin will produce a randomly-generated level-20 wand)

I appreciate you looking this over and considering it as an addition to CommandBook. I've really been enjoying having this plugin on my server since moving away from Essentials, and being able to provide wands in kits is the last piece I'm missing from that cutover. I already integrate with CommandBook for warps in a teleport spell I have, which has been working really well, so this is a logical extension for me. I just can't do it without a little API help.

Side-note: My integration with Essentials is hugely hacky (I use reflection to swap out their internal ItemDB class), I'd love to do better with CommandBook. I think this is a nice, reusable, interface that other plugins may want to take advantage of - if you have any feedback or comments, I'd be more than happy to work with you on it.

Thank you!

DarkArc commented 10 years ago

There may be a more elegant solution, I don't think we need ItemProviders. If the system is going to match purely on a name basis, then I don't think there should be the complication of providers, but I really need to think on this more. I really don't think the provider method should be in the main class though at the very least.

It's currently finals week at my university, I'll take a look at this again when I get the chance. I may also just end up write my own system for adding in custom items to the system. Of the top of the head the best solution would be to write a new component which manages "item libraries" and inject that into the inventory & kit components. The item system is definitely dusty in CommandBook, and it's something I would certainly like to improve. The current ItemUtil methods are very limited, and I feel CommandBook might be better off handling items in a dedicated component.

I'm looking into improving the Warps/Homes system as well to add per user limits, and multiple homes, I don't anticipate any API breakage there, but I figured I'd let you know. If you have any suggestions for any CommandBook API improvements in general feel free to suggest them.

NathanWolf commented 10 years ago

Well thank you for looking it over! My motivation for the provider interface (versus, say, providing a method a plugin can call to inject an ItemStack with a specific keyname) was:

  1. This allows for dynamic or random items that need to get created on demand
  2. It allows for a consolidated way for a plugin to register items (not just for kits, potentially, though I can't think of what else would use ItemUtil.getItem).
  3. The specific items I use have a tendency of getting blow away when copied, so for my purposes I'd prefer to provide the item as close to when it is given to the player to avoid these sorts of issues.

My hope is that identifying items by strings in a generic way will be conflict-free and future-proof (like whatever 1.8 brings), but it is sort of simple and inelegant, I'll admit.

Well at the very least I'm glad I got you thinking about it :) This isn't a huge deal for me, just something I'd like to support that's on my list- so I patiently await your decision. Good luck with finals!

NathanWolf commented 10 years ago

Well since this PR may turn into an "API suggestion" list, I thought I'd clarify why I put registerItemProvider in the main class, and also your notes about warps.

When I integrated, I didn't see a clear API path- so my plugin reaches pretty deep into the internals to find the component managers it needs to look up a warp. It'd be nice to avoid this.

The approach I took with my plugin's API (this may or may not work for you) was to define a core API interface that my plugin implements. I put only the most common methods in there, and then provide an accessor to a "controller" with all the weird stuff I wouldn't want to put right in the plugin class. Most of the plugin API methods are one-liners that call to the controller anyway.

I tend to agree having any functionality on the main plugin class is good to avoid, but in the case of a plugin API, it's really nice if other devs can just grab your Plugin instance, cast it to a well-known API interface and start using it.

That said, I created a simple API interface and added a getWarp method to show what I mean.

Thanks for looking :)

DarkArc commented 10 years ago

You should be able to do something like the following and get direct access to the Warps Component.

// This lookup ideally would only be done once, as it is iterative. 
// If you want to do repetitive lookups the component named based lookup would 
// be more efficient as the components are stored in a mapping.
WarpsComponent warps = CommandBook.inst().getComponentManager().getComponent(WarpsComponent.class);

World someWorld; // = someworld
String someWarp = "some warp";

warps.getManager().get(someWorld, someWarp);

Thus, you've gotten direct access to the warps component.

If you want something a bit more flexible/better looking you could create something like the following instead.

// Get an instance of the CommandBook component manager
ComponentManager<BukkitComponent> manager = CommandBook.inst().getComponentManager();

WarpsComponent warps = manager.getComponent(WarpsComponent.class);
BansComponent bans = manager.getComponent(BansComponent.class);
...

Because of CommandBook's design, it would be very problematic to provide access to functionality in the main class. Nearly all of the functionality is removable, extendable, or replaceable.

DarkArc commented 10 years ago

As for the items thing, what I'm thinking is usage of an interface such as the following:

public interface CustomItem {
    public ItemStack build();
}

Thus the item would be built by an "item constructor" each time it's requested which would be defined with your own behavior for item creation. Thus, as random as you would like. :P

NathanWolf commented 10 years ago

Ah, yes, that makes sense- a consolidated API is a bit problematic given the modular nature of the plugin. The way I currently access warps seems perfectly ok and valid, then- basically querying through the list of registered modules, makes good sense.

Ok, well, I love the CustomItem idea- so I'd be able to register one of these with a given key, and then the "build" constructor would be called as needed? Works great for me :)

Let me know if you want anything more from me, help, implementation or whatever- otherwise I'll just leave you be to do this the way you want to. I look forward to using it, but I'm in no hurry at all.

Thanks again!

DarkArc commented 10 years ago

Yup, you'd just provide the key and build would be called whenever the item was requested. :)

I'm also considering making the key a regex expression based system so you could do some cool name matching stuff, but we'll see.

Also, in case you were curious/didn't know, the ComponentManager source can be found here: https://github.com/zml2008/libcomponents/blob/master/core/src/main/java/com/zachsthings/libcomponents/ComponentManager.java

NathanWolf commented 10 years ago

That's a great idea, I'd be all for that. I'd be able to register 3 or 4 regexes instead of a zillion separate items. I have like 130 spells, and some can use hundreds of different materials... Nothing a few loops wouldn't handle I suppose, but I like your idea better.

The component manager works great for me for getting warps, disregard any API suggestions I had there :)

DarkArc commented 10 years ago

If you would like to be informed when this is implemented, follow http://youtrack.sk89q.com/issue/CMDBOOK-2388.

DarkArc commented 10 years ago

Feel free to leave any feedback or suggestions you may have on https://github.com/sk89q/commandbook/pull/104.