Add region commands

[basicmark]

Add the ability to select a region (currently requires WE support) and then run commands which will effect the selected container blocks. The aim is to allow quicker and more comprehensive set-up features especially when working on arenas built up via WE etc . The new commands are as follows:

/tchest region set [block,white-list] Create/update all treasure in the selected region /tchest region set-shared [block,white-list] Create/update all treasure with a shared inventory in the selected region /tchest region group-add [block,white-list] Add all treasure in the selected region to a group /tchest region group-remove [block,white-list] Remove all treasure in the selected region from a group /tchest region use-meta-data [block,white-list] Create/update all treasure in the selected region using the meta-data of items in the chest (sticks with names where shared will make a chest shared and any other name will add the chest to a group with that name) /tchest region delete [block,white-list] Delete all treasure in the selected region

As you can see all these commands take an optional block white-list, if this is not provided then the command will work on all the container blocks in the region and if it is then it will only work on the list provided which should be comma separated and without spaces.

[basicmark]

I hope you don't mind me adding some more commands to your plugin ;).

In case your wondering the reason I split the work into a different thread is that when operating on large selections (I was importing a 200 block radius HG map) doing that amount of processing in the main thread drops all the clients where as doing it this way I didn't get dropped when working on my test server.

I also use treasure chest for walls maps and having the meta-data option really helps as I can build one section and "pre-set-up" the chests for their different loot levels and then when I copy and paste for the remaining sections or move it then now I only have to issue one command to configure the arena (I have per arena set-up chests for each level which I use to configure to loot and then I group copy them).

[basicmark]

There is a limitation in the current implementation but I'm not sure if it's worth the effort to try and resolve. It seems that getBlockTypeIdAt will return air if the chunk is not loaded so all the chunks within the selection need to be loaded for the chests to be found.

[Mtihc]

Good stuff once again! The different thread is good. That chunk thing is fine.

But I'm not thrilled about the use-meta-data. It feels kindof hacky. I don't have any alternatives. ugh ;)

About the //FIXME's

Can you explain this comment about the region thing https://github.com/basicmark/TreasureChest/commit/b6ed7a7da98c6ae79b01d5064221f1baca40eee9#L2R277 Because I don't think I get it.

This comment about the iterator can be ignored for now. https://github.com/basicmark/TreasureChest/commit/b6ed7a7da98c6ae79b01d5064221f1baca40eee9#L2R372

This comment about RegionSelect https://github.com/basicmark/TreasureChest/commit/b6ed7a7da98c6ae79b01d5064221f1baca40eee9#L2R229 Not sure what you mean. I think you mean the SelectRegionPrompt class? That would be nice.

[basicmark]

About the use-meta-data, I know what you mean and maybe my usage of treasurechest isn't "normal". I'm happy for other suggestions, but the only other method I could think of was to have tchest copy, rotate and paste commands and based on my past experience it's very easy to get that kind of stuff wrong, plus would be more complex to code, so best to "borrow" WE and let it do the leg work :P.

Can you explain this comment about the region thing basicmark@b6ed7a7

L2R277https://github.com/basicmark/TreasureChest/commit/b6ed7a7da98c6ae79b01d5064221f1baca40eee9#L2R277

Because I don't think I get it.

That's to do with with the fact that the middle of the block is block-pos.5 so depending on which quadrant of the world you're in you may need to round up by a block to ensure the block you selected is included in the selection. You will always need to add one to both the x and z cords but you need to workout if it's at the start or end. You can see this in my EMS plugin:

https://github.com/basicmark/EMS/blob/master/src/main/java/io/github/basicmark/ems/arenaevents/EMSClearRegion.java#L59

This comment about the iterator can be ignored for now. basicmark@b6ed7a7

L2R372https://github.com/basicmark/TreasureChest/commit/b6ed7a7da98c6ae79b01d5064221f1baca40eee9#L2R372

Agreed. In fact it's the group commands which are more effected by this comment. I have a HG world setup with 200 chests and when the group forget command gets run there is noticeable lag. Having said which it's not something that happens very often and I dare say 200 chests in one group is not the norm. I'll remove this comment.

This comment about RegionSelect basicmark@b6ed7a7#L2R229https://github.com/basicmark/TreasureChest/commit/b6ed7a7da98c6ae79b01d5064221f1baca40eee9#L2R229 Not sure what you mean. I think you mean the SelectRegionPrompt classhttps://github.com/Mtihc/TreasureChest/blob/master/src/main/java/com/mtihc/minecraft/treasurechest/v8/rewardfactory/rewards/prompts/SelectRegionPrompt.java? That would be nice.

Yes that's what I was referring to. In that case would it make sense to just have a single region command and ask for the operation type in the conversion factory (something I need to look into as I've not used it before)?

Thanks for taking a look at this pull request, once I've got your feedback I'll look into making the changes

On Fri, Sep 6, 2013 at 11:34 PM, Mitch notifications@github.com wrote:

Good stuff once again! The different thread is good. That chunk thing is fine.

But I'm not thrilled about the use-meta-data. It feels kindof hacky. I don't have any alternatives. ugh ;)

About the //FIXME's

Can you explain this comment about the region thing basicmark@b6ed7a7

L2R277https://github.com/basicmark/TreasureChest/commit/b6ed7a7da98c6ae79b01d5064221f1baca40eee9#L2R277

Because I don't think I get it.

This comment about the iterator can be ignored for now. basicmark@b6ed7a7

L2R372https://github.com/basicmark/TreasureChest/commit/b6ed7a7da98c6ae79b01d5064221f1baca40eee9#L2R372

This comment about RegionSelect basicmark@b6ed7a7#L2R229https://github.com/basicmark/TreasureChest/commit/b6ed7a7da98c6ae79b01d5064221f1baca40eee9#L2R229 Not sure what you mean. I think you mean the SelectRegionPrompt classhttps://github.com/Mtihc/TreasureChest/blob/master/src/main/java/com/mtihc/minecraft/treasurechest/v8/rewardfactory/rewards/prompts/SelectRegionPrompt.java? That would be nice.

— Reply to this email directly or view it on GitHubhttps://github.com/Mtihc/TreasureChest/pull/63#issuecomment-23973710 .

[Mtihc]

Now I understand that comment about the region.. and 0.5 coordinates. https://github.com/basicmark/TreasureChest/commit/b6ed7a7da98c6ae79b01d5064221f1baca40eee9#L2R277 It's good that it's noted. I don't feel like it has priority.

You shouldn't remove the comment about the iterator. At least we know where to look, when people start asking for bigger region support or something. (tip: use //TODO instead of //FIXME because Eclipse will recognize it)

I would like to focus on that meta-data on-a-stick feature. But let's get this SelectRegionPrompt out of the way.

Uhhhmmmm.. if we start using the classes inside "rewardfactory.rewards.prompts" for other things, besides rewards. We should move the "prompts" folder up. Up up to "treasurechest.v8". If you know what I mean: The prompt classes don't reference any rewards. So they deserve to be in the main namespace.

Then we can use SelectRegionPrompt.

It uses the conversation API. new ConversationFactory(plugin) .withFirstPrompt(prompt) .withLocalEcho(false) .withModality(false) .buildConversation((Player) sender) .begin(); The first prompt should be a ##new SelectRegionPrompt(....)##... the onFinish method is overridden. That's where you get the selected region. Regardless of whether WE was used or not! The onCancel

You can look at an example here (althought this example also has some unnecessary prompts around it): From line 82 to 124 https://github.com/basicmark/TreasureChest/blob/master/src/main/java/com/mtihc/minecraft/treasurechest/v8/rewardfactory/rewards/SpawnRewardFactory.java#L83

Good luck? :/ I'm not saying you have to do this haha.

[basicmark]

I'm happy to have a go at this but can't say when a.t.m. I'll keep you posted :)

[Mtihc]

No problemo. Mind if I ramble about the meta-data feature? :)

In my own words, you can copy a region with chests in it. And then do region-commands (all chests in the region). But also group-commands ("some" chests in the region). Without that feature, you would have to add chests to groups.. after you copy. Which is a lot of extra work. I understand why you thought about built-in copy/rotate commands. But indeed.. forget about it :)

How about we use Books?!! This would be a big feature. Because we can use Books to configure more than just group-name/shared!!!

By the way... can't shared chests ALSO be in a group?

[Mtihc]

IF inventory contains book AND book title == "config" applyBookConfigTo (book, tchest)

: D

[basicmark]

Yes shared chests can also be in a group and that feature does work with my meta-data feature as does adding the chest to multiple groups as the code will scan all sticks with custom display names and thus you can add an item to as many groups as the item has inventory slots for :).

Using books is an interesting alternative approach. The advantage sticks have a.t.m. is that I can mass rename sticks (a stack at a time in an anvil) which makes it quicker to setup then creating as many books as you have chests to configure (say about 20 for a "the walls"). Now with 1.7 and book cloning using books might be more practical.

If your not against the whole idea of meta-data used to mass configure chests but rather just the use of named sticks may I ask why? Are there limitations you foresee?

[Mtihc]

It seems hacky. It will require some explenation on dev,bukkit.org. I'm afraid a lot of reactions would be: Wait what?

TreasureChest is easy to use. I like to keep it that way. I can give you a few examples where the plugin started to get weird. This feels like it would be going in that direction again. The name of a stick decides in which group the treasure is? That's not very intuitive. It's more like a sneaky workarounds, rather than a feature.

I also care about the argument you made. If it's hard to copy books, that would suck too.

[Mtihc]

Aaaaand. If we can make the Books work... that would be a very nice alternative. It opens up a lot more possibilities. You can copy regions of chests, all with different settings. Whatever the book-config feature can support (whatever is configurable)

[basicmark]

You can also copy regions of chest with different settings with the meta-data on a stick approach as well.

The way I see it we could either:

1) Drop the meta-data option until the 1.7 update in which case I would most likely hold back this entire pull request, unless you want to remove the meta-data option yourself as I'm using it a.t.m. and for now I don't think books are practical, or you could do the book thing now on in your tree after accepting this pull.

2) Support meta-data on a stick and then implement support for book-config when 1.7 is out. You could then either remove meta-data on a stick or support both together, but as you say this might be confusing.

So I guess it depends on when you where next thinking of doing a release. Although this request could always skip a release if you want to do one before 1.7.

[Mtihc]

Short answer. I just shutdown. Writing from my phone.

Im not in a hurry to release a new version.

For the sake of progress and simplicity I will merge the pull request... tomorrow.

It will be hard to convince me, to keep the meta-data feature. Call me a meta-data noob. We'll keep it until we have a proper alternative. But we will keep it undocumented if we release. Unless if I change my mind lol.

How would you add the following chest config to a stick? You can't do this with an anvil, can you? Random: 5 Unlimited: true Groups: bla1,bla2,bla3

I would feel sad somehow, if you would branch out.

[Mtihc]

I just thought of something.

If we create a command like: /tchest tobook 64 Creates a stack of books that reflect the configuration of the chest you look at.

Might as wel be /tchest tostick

Although thats harder to edit afterwards. A book can simply be opened and changed.

[basicmark]

I wasn't trying to be pushy about this and I'm sorry if I across that way. If you're not happy with a change then I'd rather not "force" you into accepting it into your code base.

What about /tchest cloneConfig 64 which clones the config book you're holding? That removes the 1.7 dependency and the permissions node will ensure normal players can't access it to dupe their books plus we can make sure it only clones books called "config".

[Mtihc]

Ohh don't worry. I was just trying to convince you. You asked for a good reason, for me to deny that feature. So I felt like I should explain. I had to convince myself too.

Your suggestion sounds good.

On Mon, Sep 9, 2013 at 8:40 AM, basicmark notifications@github.com wrote:

I wasn't trying to be pushy about this and I'm sorry if I across that way. If you're not happy with a change then I'd rather not "force" you into accepting it into your code base.

What about /tchest cloneConfig 64 which clones the config book you're holding? That removes the 1.7 dependency and the permissions node will ensure normal players can't access it to dupe their books plus we can make sure it only clones books called "config".

— Reply to this email directly or view it on GitHubhttps://github.com/Mtihc/TreasureChest/pull/63#issuecomment-24047013 .

[Mtihc]

I marked use-meta-data related methods as Deprecated. And I created a new branch called issue65. Where we can work on the book-config feature https://github.com/Mtihc/TreasureChest/issues/65

Yay branches! After the next release I will create a dev. branch

[basicmark]

Thanks :). I'll keep an eye out for the dev branch.