remove canHold check. #2119
remove canHold check. #2119
Conversation
src/Inventory.zig
Outdated
| if(sourceStack.item == null) return; | ||
| if(self.amount > sourceStack.amount) return; | ||
| if(!self.dest.canHold(.{.item = sourceStack.item, .amount = self.amount})) return; | ||
| if(!self.dest.canHold(.{.item = sourceStack.item, .amount = 1})) return; |
There was a problem hiding this comment.
I think this check can be entirely removed. The code that follows it seems to loop over every slot in the inventory and attempt to transfer as many items from the source as possible, it doesn't look like it will fail even if there isn't any room at all. I could be wrong though
There was a problem hiding this comment.
I see your point. You are right that it doesn't fail. I mean even before if we had as an example one full stack and then a second one which was almost full. It would execute a move operation on the full stack with amount 0 and then a move operation with whatever amount for the second slot. So now we just do it for every slot that is the same item. So at the end it is a question do we want to to make a move operation with amount 0 to all slots with the same item or do we just want to do a before check.
There was a problem hiding this comment.
Couldn't we just remove the check even?
There was a problem hiding this comment.
yes you are both right. I thought this could run the move command a lot. But my mind just ignored the if(remainingAmount == 0) break; line. So Fixed
There was a problem hiding this comment.
I think you are still right, in the case that the destination is nearly full of the same type of item like this: (Each x is the same item at max stack size)
[x][x][x][x][x][x][x][x][x][x]
[x][x][x][x][x][x][x][x][x][ ]
the code will call cmd.executeBaseOperation with an amount of 0 on each of those full slots. I have no idea what the performance/network overhead of that is so it might not be worth it to fix. A if(amount == 0) continue; would do the job though.
There was a problem hiding this comment.
I was first not certain if this is realy send to the server, so I tested it. Yes we are sending 0 amount move operations to the server. I sadly had not enough time to add the one line. But I will do it when I am back at my PC. In my opinion at least every network overhead that can be removed should be removed.
Wunka
changed the title
|
While we are not working on the same thing, we are working on almost the same lines so just wanted to mention that #2190 could create conflicts |
3 participants
Fixes: #1881
just a little change that makes the amount requirement of shifting to an inventory not be the stack size