Help with auto-ejecting items

[TesterTesting135]

Hello, so I've made a cobblestone generator but would like it to auto-eject items. This is my method for doing so:

 

private void sendItems() {
    if (!outputHandler.getStackInSlot(0).isEmpty()) {
        for (EnumFacing facing : EnumFacing.VALUES) {
            TileEntity tileEntity = world.getTileEntity(pos.offset(facing));
            if (tileEntity != null && tileEntity.hasCapability(CapabilityItemHandler.ITEM_HANDLER_CAPABILITY, facing.getOpposite())) {
                IItemHandler handler = tileEntity.getCapability(CapabilityItemHandler.ITEM_HANDLER_CAPABILITY, facing.getOpposite());
                if (handler != null) {
                    for (int i = 0; i <= handler.getSlots() ; i++) {
                        handler.insertItem(i, outputHandler.getStackInSlot(0), false);
                        outputHandler.extractItem(0, 64, false);
                        if (outputHandler.getStackInSlot(0).isEmpty()) {
                            break;
                        }
                    }
                }
            }
        }
        markDirty();
    }
}

but for some reason, it will only insert items to 1 slot of the chest. Any idea why?

Thanks.

[TesterTesting135]

Ok, thank you!

[TesterTesting135]

Ok so i am using insertItemStacked, and i modified the code a bit. Now it works with some tile entities, like a bin from mekanism and a vacuum chest from ender io. However, for other tile entities, like the chest, it will not work, and won't even insert into the container. Here is my code:

private void sendItems() {
    if (!outputHandler.getStackInSlot(0).isEmpty()) {
        for (EnumFacing facing : EnumFacing.VALUES) {
            TileEntity tileEntity = world.getTileEntity(pos.offset(facing));
            if (tileEntity != null && tileEntity.hasCapability(CapabilityItemHandler.ITEM_HANDLER_CAPABILITY, facing.getOpposite())) {
                IItemHandler handler = tileEntity.getCapability(CapabilityItemHandler.ITEM_HANDLER_CAPABILITY, facing.getOpposite());
                if (handler != null) {
                    ItemStack result = ItemHandlerHelper.insertItemStacked(handler, outputHandler.getStackInSlot(0), false);
                    if (result.isEmpty()) {
                        outputHandler.getStackInSlot(0).shrink(64);
                    }
                    if (outputHandler.getStackInSlot(0).isEmpty()) {
                            break;
                    }
                }
            }
        }
        markDirty();
    }
}

Also, what do you mean by the get capability returning null? I checked if it had the item handler capability first.

[TesterTesting135]

Could you please bring me a link to the documentation?

[TesterTesting135]

Is this the correct webstite? mcforge.readthedocs.io/en

[TesterTesting135]

I can see the code, but what do yoiu mean they have java docs?

[TesterTesting135]

Ok, thanks for showing me. Unfortunately, i'm not getting the same thing

.https://imgur.com/a/2xiJfE9

[LexManos]

Don't call hasCapability only to then immediately call getCapability. getCapability will return null if the capability is not present (which you already check for...).

 

That's actually the exact opposite of how things were designed. Get is costly, has should be fast

 And thus better to run. But whatever nobody ever implemented it right.

[Draco18s]

6 hours ago, LexManos said:

Don't call hasCapability only to then immediately call getCapability. getCapability will return null if the capability is not present (which you already check for...).

 

That's actually the exact opposite of how things were designed. Get is costly, has should be fast

 And thus better to run. But whatever nobody ever implemented it right.

I would swear that when I went looking, Forge (that is, in the vanilla patches) didn't use hasCap anyway (it did the null check). That was a while ago, so I could be wrong.

 

In any case, d7 is basically right. The logic in the two functions was the same, and I never saw a reason to duplicate that code. I suppose the logic could have been in hasCap and getCap would query hasCap to decide whether or not to return...except that doesn't help when you have multiple capabilities that are of the same type but discriminated by side. So back in getCap the logic went.

[TesterTesting135]

I'm using forge 1.12.2-14.23.5.2836 and mappings "stable_39". I just downloaded the mdk, extracted it, and hopped into intellij and imported the project. I ran setUpDecompWorkspace.

[LexManos]

The IDEA is to have complex logic in get, and lightweight logic in has.

As in:

has(cap, side) {
  if (cap == MYCAP) return true;
}
get(cap, side) {
  if (cap == MYCAP) {
    if (side == UP) {
      if (myUpHandler == null) {
        myUpHandler = superComplexMethodThatCalculatesMyUpHandler();
      }
      return myUpHandler;
    } else {
      if (myOtherHandler == null) {
        myOtherHandler = otherSuperComplexCodeThatTakesTime();
      }
      return myOtherHandler;
    }
  }
}

We now have that complex logic in the Supplier for the LazyOptional in 1.13+.

But people are STILL not doing it right...

[Draco18s]

3 hours ago, LexManos said:

has(cap, side) { if (cap == MYCAP) return true; }

Is hasCap supposed to return true for a given capability, even if that capability is only exposed on one side?

3 hours ago, LexManos said:

if (myUpHandler == null) {

Also, why would you check this here? Wouldn't you create the handler in the TE constructor?

3 hours ago, LexManos said:

superComplexMethodThatCalculatesMyUpHandler();

Also no one (that I know of) does this. The worst case I know about is doing CombinedInventoryWrappers and that's usually only for when the side is null (block breaking covering 90% of cases where it would be called with a null side). And if the Combined wrapper is really that expensive, then fine, I can cache it, no problem.

[LexManos]

5 minutes ago, Draco18s said:

Is hasCap supposed to return true for a given capability, even if that capability is only exposed on one side?

Does my get only return a cap if its on a specifc side? No it returns caps for all sides. What did you expect the code to look like? 
 

if (cap == MYCAP){
  if (side == UP)
    return true
   else
     return true
}

You know what that condenses down to?

Are you starting to see what I mean by lightweight code?

 

5 minutes ago, Draco18s said:

Also, why would you check this here? Wouldn't you create the handler in the TE constructor?

https://en.wikipedia.org/wiki/Lazy_initialization If nobody wants your cap, why bother creating it?

5 minutes ago, Draco18s said:

Also no one (that I know of) does this. The worst case I know about is doing CombinedInventoryWrappers and that's usually only for when the side is null (block breaking covering 90% of cases where it would be called with a null side). And if the Combined wrapper is really that expensive, then fine, I can cache it, no problem.

Who knows, caps can be anything. Cap initialization can take whatever values you want. What about say.. a cap that keeps track of all entities within a BB around the TE. Instead of having every BB list tracked for every chunk every tick. You could delay capturing that snapshot of entities until the getCap is called. And it could add it's chunk event handlers lazily. I dont know, thats not really a good example but my point is. Building the caps COULD be expensive both CPU wise and memory wise. You can use lazy initialization in your get to save those costs.

[Draco18s]

1 hour ago, LexManos said:

Building the caps COULD be expensive

That I can agree with, its just that for the vast majority of use-cases the caps are something that are existent before "anyone" calls for it because it's used by the TE itself (inventory, energy, etc). And that's why "everyone uses it wrong." They use it "wrong" because the performance difference is so negligible as to be irrelevant. Then, if you're going to call has() followed immediately by get() if you're interested in a Thing, its going to need to be constructed anyway. And if Thing wasn't meant to be provided under the conditions you queried, it won't get created.

 

1 hour ago, LexManos said:

Does my get only return a cap if its on a specifc side?

I wasn't asking about your specific chunk of pseudocode. I was trying to ask in general. The "I see what you did there, but what about x?"

 

In any case I am just trying to get a handle on the reasoning behind the design decision; I've met you in meatspace, I know you're not this grouchy, so stop taking everything I say as a personal attack. But I'm going to have to disengage.

Edited August 2, 2019 by Draco18s

[LexManos]

It's a stupid simple concept. It was literally designed to replace these instanceof/cast in a non-hard dep way.

has == instanceof

get == cast.

If has == true

Then get using the same cap&side MUST not return null

I hate people doing `if has(){ cap = get(); if (cap != null)` because thats not how it was designed. 

 

I'm not taking everything as a personal slight, I just stated that Dies was spreading the exact opposite of what to do.

So I informed you of what the design was. {As I have informed people many, many, MANY times since the system was designed}

 

 

This entire system is moot however, as 1.13+ now uses LazyOptionals because people couldn't understand the concept of instanceof/cast.

People are NOW being stupid and returning new instances of the LazyOptional every get call.. and that's just even more stupid...

 

It's a simple system, with TONS of examples in Forge/Vanilla. how can people screw it up so much?

[Draco18s]

9 hours ago, LexManos said:

It's a stupid simple concept. It was literally designed to replace these instanceof/cast in a non-hard dep way.

Yes. I know that. That wasn't my question. My question was why the separation between hasCap and getCap even existed at all.

 

9 hours ago, LexManos said:

It's a simple system, with TONS of examples in Forge/Vanilla. how can people screw it up so much?

Because 95% of mod makers aren't people with a decade of professional experience learning from people with an equivalent amount of experience (and from a decade of experience, I can assure you that even that isn't sufficient sometimes) as evidenced by the number of people who come here asking for help and say they've "been following LoreMaster's Youtube tutorials" or "using MCreator" or similar.

 

9 hours ago, LexManos said:

This entire system is moot however, as 1.13+ now uses LazyOptionals because people couldn't understand the concept of instanceof/cast.

People are NOW being stupid and returning new instances of the LazyOptional every get call.. and that's just even more stupid...

See prior comment.

 

In my own code I did it because I hadn't done a clean up pass on the code yet. I did a quick-and-dirty get-things-working implementation that I knew was not fully compliant with good practices. My focus was on re-implementing existing mechanics, not optimizing performance. Hell, I wouldn't have even thrown my crap up on GitHub if it weren't for the fact that I had an unrelated issue (invalid data files causing all data files to fail). If it really bothers you that much, I've now fixed it.

 

Regarding the old system, what I was agreeing with, and what d7 actually said, emphasis mine:

 

On 8/1/2019 at 2:51 AM, diesieben07 said:

On 8/1/2019 at 2:03 AM, LexManos said:

But whatever nobody ever implemented it right.

Yes, except I don't know how you expect[ed] people to implement this [correctly].

No one did it right, despite the Forge-supplied examples because: (1) no one looks at Forge supplied examples, (2) they're lazy, and (3) they don't care.

And my addendum was (4) even if they did they would find places where hasCap was never called anyway leading to confusion about what it was for and why it existed and people continuing to implement it in the getCap!=null way.

 

Examples? Well I said I wasn't sure if I was remembering correctly. But sure, let me go look...

Here's one (1.12.1 build 2462; because that's what I had built an old mod against and had readily available).

net.minecraftforge.client.model.animation.AnimationTESR:

                IAnimationStateMachine capability = te.getCapability(CapabilityAnimation.ANIMATION_CAPABILITY, null);
                if (capability != null)

 

I'm trying to understand the separation between hasCap and getCap. You have more experience than I do, you designed it, but its really frustrating for me to communicate with you.

[loordgek]

draco you never invalidate your lazycaps

https://github.com/MinecraftForge/MinecraftForge/blob/1.14.x/src/main/java/net/minecraftforge/common/util/LazyOptional.java#L293

[Draco18s]

41 minutes ago, loordgek said:

draco you never invalidate your lazycaps

https://github.com/MinecraftForge/MinecraftForge/blob/1.14.x/src/main/java/net/minecraftforge/common/util/LazyOptional.java#L293

I did not know this was a thing.

Thanks.

[LexManos]

3 hours ago, Draco18s said:

Yes. I know that. That wasn't my question. My question was why the separation between hasCap and getCap even existed at all.

I'm trying to understand the separation between hasCap and getCap. You have more experience than I do, you designed it, but its really frustrating for me to communicate with you.

 

13 hours ago, LexManos said:

has == instanceof

get == cast

There are many instances where you don't give a shit about the actual content of the cap. You just want to know if it IS a thing.

For example pipes, you don't care when you render the pipes what exact items are inside a block.

Just that that block has a inventory, and you should render that side being connected.

You don't want to spend the massive amount of time every frame getting the large object as a return value and checking null. When you could just call a SIMPLE implementation that tells you directly the information you want to know.

 

Hell even more reason to have the items be lazy initialized is because the client doesn't need to know, and have in memory, the inventory of a block. Because it would always be useless/empty unless you specifically add a sync packet that sends over all the inventory contents.

 

Is that simple enough for you to understand?

[Draco18s]

Yes, that is quite informative.