Botania

Botania

133M Downloads

[API] IBlockProvider `provideBlock()` return value poorly documented

gr8pefish opened this issue ยท 4 comments

commented

I'm trying to implement the IBlockProvider for my mod, and the doIt parameter in the provideBlock method was only firing once as 'false', when it should have been true, which was messing me up. If I removed the if (doIt) {//code} clause then it fired twice, removing two blocks from my inventory each time. Oddly enough, changing it to if (!doIt) seems to fix it (as it only fires once) and gives all of the intended functionality, but it feels wrong to do so.

Here is my semi-cleaned up code for using it.

Keep this in mind:

  • Ignore the commented out API annotations, they don't work because I haven't figured it out (if you know what I'm doing wrong with that a quick reply would be appreciated by the way) but they shouldn't be affecting it.
  • If you want a dev or normal .jar version of what I have just ask and I can build one for you so you can test/debug.
  • If you have clarification questions please ask. I know I haven't explained it fully, and that is because it is late and I am tired so I don't fully understand what is happening with your code.

Note: I was only testing with the ItemExhangeRod/Rod of the Shifting Crust, as I think that is the only item that looks for and uses IBlockProviders.

P.S. Has any other mod you know of correctly implemented it?

Edit: Here is where you can see the debugging Logger lines that I used to figure this out.

commented

I think that doit is "use mana", bad a name as that may be. You shouldn't need to use it at all if it's just a backpack mod.

This seems a nonissue, as you thought doIt did something other than what it does.

commented

And also, since there is no unified external inventory, in my mod I am just saving the "equipped backpack" as extra data on the player, meaning there is no reason why you would check for this when looking for a IBlockProvider, which is kind of an issue.

To circumvent this, I suggest you can have a method that is something like
public ItemStack addAdditionalIBlockProvider() to the API where any odd instances of IBlockProvider can be accounted for easily.

You can then add the additional IBlockProviders (if any) in to the array here.

commented

I think you are mistaken. In the API comments it says "The doit paremeter specifies whether this is just a test (false) or if the item should actually be removed (true)." And if I implement your 'fix' of removing the doIt check then it fires twice and pulls two items out of the backpack.. Thanks for trying though :)

I think I was actually on the right track with the !doIt, strange as that may be. Or maybe I'm wrong.

If we look through the ItemExchangeRod we see that, on left click, the rod tries to exchange, and if it can it continues and does so. The relevant method in exchange, the one that actually calls provideBlock is here as a 'checking/test call' with doIt being false, and then if it can do so it calls the method again then here with 'doIt' as true. But that obviously isn't working quite as intended or my original implementation of IBlockProvider would have worked...

Here are both of the relevant classes:
ItemExchangeRod
IBlockProvider

commented

Okay, so I figured it out! The key is here, which is returning true if the amount is greater than 0, even if doIt is false.

Here is my new code showing working usage.

@Vazkii I would recommend adding a little something to the comment in the API regarding that return value usage. I think it is best if you just do it, it seems too small for me to do a PR.

Also, if either of you know how to implement the @Optional fml annotation I wouldn't mind some help on that ;) Fixed: The modid is "Botania" not "botania"