Open VirgilCore opened 3 years ago
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!
Worth noting the crash (and the same problem) occurs in getDrops
.
Probably changing it to IEnergyStorage
should fix it. No idea why the code is so specific about the energey storage it gets 🤔
@MrJake222, there's no setEnergyStored
method in IEnergyStorage
probably
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
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
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.
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
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
.
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.
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).
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
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.
As you wish ¯_(ツ)_/¯ It's either code duplication or additional field for each capacitor storage object anyway
Btw, what's the point of StargateAbstractEnergyStorage.receiveEnergyInternal?
It should be able to bypass the limit.
crash-2021-05-24_20.35.20-server.txt This causes complete crash