Capacitor crashing server

[VirgilCore]

crash-2021-05-24_20.35.20-server.txt
This causes complete crash

[slava110]

Yep, there's a problem
https://github.com/MrJake222/AUNIS/blob/master/src/main/java/mrjake/aunis/block/CapacitorBlock.java#L99
It's trying to cast item energy storage to abstract energy storage. To be fixed.
Thanks for reporting!

[MrJake222]

Worth noting the crash (and the same problem) occurs in getDrops.

[MrJake222]

Probably changing it to IEnergyStorage should fix it. No idea why the code is so specific about the energey storage it gets 🤔

[slava110]

@MrJake222, there's no setEnergyStored method in IEnergyStorage probably

[MrJake222]

It's breaking in getDrops because of this change.

To me StargateItemEnergyStorage should extend StargateAbstractEnergyStorage (it's intuitive because it's a type of energy storage for Stargate) and handle writing to stack's NBT tag in overridden methods

[slava110]

Well... You'll need to override everything then. Because StargateItemEnergyStorage should modify ItemStack nbt, not energy field from EnergyStorage. And we'll get useless field in this object

[MrJake222]

I think we could get away with just overriding onEnergyChanged and getEnergyStored. It would be more concise after all with the rest of the codebase. energy field is private so it wouldn't matter that much. getEnergyStored could update it and then return the result of a super method.

[slava110]

Maybe... But if there's no actual reason for StargateItemEnergyStorage to extend StargateAbstractEnergyStorage other than code intuitivity then we can just rename this class/move it to capability package. Might be even more intuitive cause it's more like CapacitorEnergyStorage, not something related to stargate itself
Also leaving random unused fields in objects isn't good practice probably
I'd agree if we had hierarchy like StorageClassWithField extends AbstractStorage and StorageClassWithItemStack extends AbstractStorage. But we're extending forge EnergyStorage in StargateAbstractEnergyStorage so... Ye

[MrJake222]

The field technially wouldn't be unused because it'll store the energy and pass it around for saving to/restoring from NBT.

The Gate essentially treats itself as one of the capacitors right now so the border between the capacitor and the Gate as a energy buffer is very thin. The naming is probably lacking here because StargateAbstractEnergyStorage essentially is EnergyStorage for Aunis with setEnergy method.

[slava110]

Well, yes, basically variable duplicate. Power will be stored both in NBT and this variable and all operations will work with 2 similar variables (NBT and field). But... What for?

The Gate essentially treats itself as one of the capacitors right now so the border between the capacitor and the Gate as a energy buffer is very thin

But it doesn't treat itself as capacitor based on ItemStack, you know

The naming is probably lacking here because StargateAbstractEnergyStorage essentially is EnergyStorage for Aunis with setEnergy method

If you just want to have setEnergy method both in capacitor and stargate storage we can just add this method to capacitor storage class, you know. If you want to use upcast for something - we can add interface. Also it's possible to create custom implementation of forge IEnergyStorage as base class for other capacitors. But currently upcast isn't actually needed anywhere so we can just add setEnergy method to capacitor storage class. Should be enough. And then just rename/move this class to capability or item package so it'll not look like it's a part of StargateEnergyStorage hierarchy.

[MrJake222]

But still there will be a lot of code duplication handling exact same things (simulations when extracting/inserting energy for example). One int variable is only 4 bytes and it's already needs to be created for the sake of reading the data from NBT (it'll be local not global to the class but still).

But... What for?

For simplicity. You wouldn't need to think whether the returned class will be ItemStorage or StargateEnergy. I get it, the naming is not perfect, but the base class don't have too much overhead.

But it doesn't treat itself as capacitor based on ItemStack, you know

The only real difference is where the data is being saved. For ItemStack it's the compound attached to ItemStack, for TileEntity it's the compound saved with writeToNBT and it more or less happens every time the energy changes (mind the game is not saving it constantly).

[slava110]

But still there will be a lot of code duplication handling exact same things (simulations when extracting/inserting energy for example).

That's why we can create custom base class if you want.

One int variable is only 4 bytes and it's already needs to be created for the sake of reading the data from NBT (it'll be local not global to the class but still).

Globals will not be collected by garbage collector tho so I think it's better to have local vars

For simplicity. You wouldn't need to think whether the returned class will be ItemStorage or StargateEnergy

Well, if you're working with item it will return item storage, if you're working with stargate - stargate storage. Isn't it simple? Also we can create IEnergyStorageModificable interface if you want upcast / common base class without forge EnergyStorage in hierarchy

The only real difference is where the data is being saved

Yes. And if it's the difference - it's probably better to have 2 different classes with different read/write methods extending one base class without read/write method

[MrJake222]

have 2 different classes with different read/write methods extending one base class without read/write method

The base class doesn't have read/write methods. There is only serialization which can be ignored.

That's why we can create custom base class if you want.

Or just rename StargateAbstract to sth else.

[slava110]

As you wish ¯_(ツ)_/¯
It's either code duplication or additional field for each capacitor storage object anyway

[slava110]

Btw, what's the point of StargateAbstractEnergyStorage.receiveEnergyInternal?

[MrJake222]

It should be able to bypass the limit.