Useless Calls
Speiger opened this issue ยท 27 comments
https://github.com/BuildCraft/BuildCraft/blob/7.1.x/common/buildcraft/core/lib/fluids/TankUtils.java#L51
This call is totally useless. PlayerInventory mark this as inventory changed but they actually do never change the inventory.
change the code to: player.inventoryContainer.detectandselectchanges()
That fixes by the way the bug with filled buckets disappear in the inventory until you open any other inventory then the players inventory.
@AEnterprise is this fixed yet?
@Speiger this is no longer an issue with the fluid and inventory capabilities, this is only a problem on 1.7.10 but there are no other outstanding fixes for that and i don't think a new version will be released just to fix this one (relatively minor) bug
@AEnterprise Forge doesnt even with 1.10.2 update the item inventory properly. Since its not a OpenInventory & MarkDirty calls of any kind are useless at those things means that you transfer that bug just over...
So its not a 1.7.10 only bug. We talk here about proper ItemSyncing when changing the inventory in the one and only case where it is not handled properly... So do not say its fixed until you show me a live video with the fix in it or the fix that does it properly...
@AlexIIL just a question does whenever you fill a container with a BC Tank call EntityPlayer.openContainer.detectAndSelectChanges()? Because EntityPlayer.inventoryPlayer.markDirty() is 100% Useless
@Speiger tanks have been rewritten for 8.0 so it should be fixed as inventory desync is very noticable during testing, if it turns out it's not completely fixed it will be handled then
@Speiger currently it uses FluidUtil.interactWithFluidHandler so if their is a bug in that it will be forge's concern, not ours.
@AlexIIL Not if it is used on a Players Inventory... Since there is no automated syncing like on all other containers... So players are a thing you need to take care of manually... And this is not a forge bug...
If forge would automate these things then normal Inventories would cause way more lag then usual if an action does happen more then once at the same time... (Doubling packets if it is not nessesary)
@AEnterprise IC2Classic is 1.10.2 too and uses FluidUtil also. And when i checkt its code i found no code at all doing proper updating of the Players Inventory... So Again: Players Inventory when no gui open that is not the normal hud or players inventory alone then player.openContainer.detectAndSelectChanges needs to be called... (Also just as a Note: Yes Forge calls MarkDirty on the Players Inventory when interact with fluidHandler but markDirty is a useless call since its change a field that is not handled at all)
InventoryPlayer Class
public void markDirty()
{
this.inventoryChanged = true;
}
/**
* Set true whenever the inventory changes. Nothing sets it false so you will have to write your own code to check
* it and reset the value.
*/
public boolean inventoryChanged;
Here is the thing whenever markDirty on the Players inventory is called except that nothing happens...
So do not tell me again that i do not know what i am doing until you can not proof me wrong. Because this bug (inventory not updating inventory properly when filling a cell with rightclick on a tank) exists ages. And we talk here about 1 Extra line that works for 100% and can fix this kind of bug since ages...
(And i have tested that long enough to ensure you that)
Edit: This bug still exists because of lazyness. And telling "If its broken then its a forge bug" is based on even more lazyness... (And here i am one of the laziest persons on this world but still have not this bug)
@Speiger we are talking about interacting with tanks in the world, there can't be an open GUI then and even if so if fluidutil doen't do that while you think it should, take it up with forge
@AEnterprise i am talking about interacting with tanks in the world without gui all the time there was no topic change since then... And i think you do not know how Minecrafts ContainerClass work... Because that is the only sync in the PlayersInventory... And that does still apply when no gui is open... So you need to call the container on server side and say: Sync because the player does that only if he has a NonePlayerInventory/Hotbar Gui Open...
what i ment is that there shouldn't be any custom gui's open, we just call forge code to get that bucket (or whatever) filled, so forge is in charge here of making sure stuff doesn't desync
Yeah but then you have the: Items not showing up in your Inventory until you open any gui that is not a PlayerOnlyBased Gui... And to fix that you need to call after the item is injected to the player: player.openContainer.detectAndSelectChanges()...
@Speiger forge does the filling of the item so forge should do the syncing, go bug them about it
@asiekierka can you lock this as it keeps going in circles?
@AEnterprise stop pushing bug to the Others. If they would do that for everything that would cause way more issues then just you exchanging 1 Line of code... So Stop playing dumb....
@asiekierka when i talk to him i think talk to a wall...
(AEnterprise when you do not want to fix your bugs and push it to someone else that does not make you a good coder... Actually it decreases your competence drasticly... And Even asie did say: this is an Important issue)
i am pushing it away because 1.7.10 is no longer actively developed, all work is on the 8.0.x branch and there BC no longer fills buckeks but forge handles it
you are correct that in the PAST it was a BC bug but it is no longer the case
not even gona reply to the personal insults
@AEnterprise as i have told this is not a 1.7.10 only bug... This is a Minecraft Consistent thing... You can get it between 1.7.10 & 1.10.2 at any time... So please stop playing dumb...
insults? I am harsh i know that but that wasnt a insult... not even personal... I told you what you were showing me here...
Yes... But forge but since you call Forges thing there. But what stops you by making it to this:
@Override
public boolean onBlockActivated(World world, BlockPos pos, IBlockState state, EntityPlayer player, EnumHand hand, @Nullable ItemStack heldItem, EnumFacing side, float hitX, float hitY, float hitZ) {
TileTank tile = (TileTank) world.getTileEntity(pos);
if(heldItem == null || tile == null) {
return super.onBlockActivated(world, pos, state, player, hand, heldItem, side, hitX, hitY, hitZ);
}
boolean result = FluidUtil.interactWithFluidHandler(heldItem, tile.getCapability(CapabilityFluidHandler.FLUID_HANDLER_CAPABILITY, null), player);
if(result) player.openContainer.detectAndSelectChanges();
return result
}
This would fix the bug. Since you guys call the function in the players inventory.
But this is lazyness & not caring about fixing bugs. And since you are using a function that is partly broken does not allow you to say: "Its their bug" because you knew that their side was broken too...
(Even if you didnt know that. It would have showed up and still it does not give you the right to say that...)
@Speiger you don't seem to know the new code involved so please look at that before making more comments on this.
BC doesn't fill buckets or fluid containers anymore when someone clicks on a tank, forge does that. So once someone clicks on a tank the code that is executed isn't BC code but forge
Well on a Intigrated server its unlikely to happen.
Since most syncing falls away.
And the bug actually can happen with plain items.
Best thing could be something that stacks as filled form. But it could be that since they added the second hand that the sync destructions are much higher.
Now to your ()
Let's say you have 4 item changes per tick on the same slot. If you call detect&select changes on each of them you have 4 packets. But if you do it only once per tick (like the EntityplayerMP does when a none player based GUI is open ) then you save 3 packets
Also I have no issues to be proven wrong on this issue. It just shows me: you did your job right
@Speiger As far as I can tell this bug doesn't exist in 8.0.x when testing with a vanilla bucket and a BC tank. Does this bug require a different sort of bucket that can stack or changes its NBT or something like that?
(Also note that "inventory.detectAndSendChanges" looks like it won't send multiple change packets so I don't have an issue adding a call to it if it requires that).
@asiekierka that also fixes the items do not show up when you click on the Tanks bug..
Edit: i can not believe that this is not fixed yet. Its a small 1-5 minutes bug
@Speiger it's a small simple thing that you seem to find important. Why don't you make a PR? saves the BC devs time and your bug gets fixed
Actually, it is important, it's just that between major game-breaking bugs and desperately missing features the time to research the implications of a change is hard to find. Or, rather, was hard to find back when I was the only one still working on it.
@AEnterprise simply because when start something like that then i would start absorbing the whole source of BC which would make result into way more work then it should be... Which would slowdone the progress of my own work... So yeah thats why... (I discovered that bug when i had my own BC Version @AEnterprise in that time...)
@AEnterprise since Capabilities are a thing mark dirty is no longer nessesary... Only thing is that code has to be changed to: player.openContainer.detectAndSelectChanges()... Except you want to keep the bug that fluidcontainers that get filled do not show up until you open up a new inventory (that is not the players inventory)