SuperMartijn642 / WirelessChargers

3 stars 0 forks source link

[Bug] BlockEntity energy transfer attempted on client #20

Closed Linguardium closed 1 year ago

Linguardium commented 1 year ago

Version Info

Description of the Bug Wireless Chargers charging things on clientside when run on servers. Likely not an issue singleplayer since some data objects are shared in integrated servers

looks like wirelesschargers is performing actions on the clientside (inserting energy) which should not happen clientside.

I would assume this was found due to a change in the energy api 2.3.0 (but that change is kinda old) that allows clientside lookup. Wireless Chargers should have a client and server tick method (it does not) instead of sharing 1 method or should check isClient before manipulating block entity data

https://github.com/SuperMartijn642/SuperMartijn642sCoreLib/blob/66c28db873047526cc68f4c635b1fdbcc18a0ab8/src/main/java/com/supermartijn642/core/block/EntityHoldingBlock.java#LL30C13-L30C13 wireless charger uses core lib, corelib calls update for client and server ticks. https://github.com/SuperMartijn642/WirelessChargers/blob/822af1015dda88ac9dc60eda5bf413cafc3c2b28/src/main/java/com/supermartijn642/wirelesschargers/ChargerBlockEntity.java#L109 wireless chargers finds energystorage of indrev on clientside and attempts to charge it since energy is on both sides

This ultimately should not be happening clientside. the check (canInsert or getMaxInsert) does not have server/client awareness and so it should not be used to determine if actually transferring should happen. Client shouldnt change data, only representation.

https://github.com/GabrielOlvH/Industrial-Revolution/blob/master/src/main/kotlin/me/steven/indrev/blockentities/storage/LazuliFluxContainerBlockEntity.kt#L92 indrev energy storage block that can be used to test the issue.

SuperMartijn642 commented 1 year ago

Could you provide a reason as to why this shouldn't be done on the client? Is this causing any issues?

On Forge, I have run into issues where some blocks from other mods don't always sync there energy as they expect the same transfer to occur on the client.

Linguardium commented 1 year ago

logistically, the client should not be updating its own data (except in cases where the server sends updated nbt datat to the client) since that can and does cause desyncs with the serverside data. The client should be updated via the nbt routines (or more specifically the getUpdatePacket method which I normally see just call getUpdateTag or saveWithoutId routines when implemented). This is how vanilla does it and how the fabric block entity routines are set up.

If forge mods are not passing the data back to the client when the server updates it, then perhaps they dont actually visually display this data on the client. The client doesnt NEED to know how much energy is in a block unless the mod does something with that data.

yes, both tech reborn and industrial revolution crash when you pass energy to or from their blocks on the client (in certain circumstances) as they proceed to attempt to send this updated datat to the client using a server method...which wont work on clients. If you look at the indrev code i linked, the lazuliflux energy storage will call update() when a transaction is committed to sync the data back to the client, which crashes.

Arguably you can prevent the crash on the clientside in each individual mod but the logic behind changing block data on the client side alone is flawed since the server is the truth in source of the data and the client's job is to display the data it receives from the server.

edit: as mentioned, this only really became a problem in 2.3.0 of the energy api as that was when clientside was allowed to actually interact with it, as a way to provide better connectivity of wires and such (and maybe some other niche things where knowing what could connect and pass transactions was needed)

Lgmrszd commented 1 year ago

This behavior actually causes crash with IndRev, see this log https://gist.github.com/Lgmrszd/67085d2928703e20bf3dfd4b973549d5

SuperMartijn642 commented 1 year ago

indrev

Whoops did not realize that was referring to another mod 😅

After looking into ChargerBlockEntity#update I get the issue now. Not entirely sure what I was doing as rendering specific stuff was interleaved in there as well. Might just have been me not paying attention whenever I wrote it.

Should be fixed now in version 1.0.9 of Wireless Chargers. Thank you both for reporting the issue!