Reborn Core

Reborn Core

91M Downloads

[Any Version] Things that need to be cleaned up

Speiger opened this issue ยท 3 comments

commented

Warning: A lot of text and a lot of code references. I tried to be nice. (Keep the "tried" in the mind).
And these things are examples you can find in almost every class.

1: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/tile/TileMachineBase.java#L69
Could be getBlockType since its cached and does not call getBlockAll the time.
2: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/tile/TileLegacyMachineBase.java#L159
Double call for the same thing.
3: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/tile/TileLegacyMachineBase.java#L80
Again: getBlockType to call cached data so its not using the more laggy getBlock function since the blockType doesnt change in a TileEntity. Unless you have 2 blockIDs sharing the same block but then again it caches it anyway again.
4: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/tile/TileLegacyMachineBase.java#L69
Double call for a thing every tick. Its basicly a constant casting check all the time. That could be cleaned up a lot.
5: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/tile/TileLegacyMachineBase.java#L55
Synchronizing the Inventory but not important networking data. Since the Container syncs the inventory anyway you are sending useless data over just to sync some functions. I suggest to make a seperate function for that or ignore it.
6: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/tile/TileLegacyMachineBase.java#L50
Same as 5.
7: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/blocks/BlockMachineBase.java#L113
why are those things still existend? You need blockpos in 95% of the cases anyway.
8: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/blocks/BlockMachineBase.java#L183
Look what the ItemStack.copy function does.
9: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/blocks/BlockMachineBase.java#L212
first a instance Check includes a != null check. Second you basicly waste CPU to check if it is a machine base. Why not adding into the block a function like this: canRotate(IBlockState) that saves CPU since you need to access the IBlockState in the first place and second you can have the block & not the TileEntity decide how the facing is handled.
10: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/blocks/BlockMachineBase.java#L61
Why??? That is just overcreating properties and using more memory for every block you create with that class. Since its actually beeing used by the IBlockstateContainer the garbage collection can not remove these classes and you are creating new stuff with this.
11: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/blocks/BlockMachineBase.java#L366
What about a Util class that contains all these kind of function?
12: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/multiblock/MultiblockRegistry.java#L24
that is not known commonly but containsKey = get()!=null. So you have 2 getCalls i suggest to put a get instead of contains key and then make a != null check.
13: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/multiblock/MultiblockTileEntityBase.java#L305
Em what about a EnumFacing for loop?
14: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/powerSystem/TileEnergyBase.java#L99
Double call again.
15: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/powerSystem/TileEnergyBase.java#L149
Useless check since you use math min.
16: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/powerSystem/TileEnergyBase.java#L354
Unless someone hacks into it and tries to drain energy based of EU this is something that is not needed and just adds lag to the Enet. IC2Exp is already not the best in performance in the Enet but these things make it worse. The reason why its not needed: The enet ignores this function to 100% until you add your EnergyTile to the Enet.
17: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/powerSystem/TilePowerAcceptor.java#L34
Unless its not a powerstorage that also emits power this is 100% something that creates more lag for the IC2Enet because there are more tiles to check every tick.
18: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/powerSystem/TilePowerAcceptor.java#L61
Why dont you make a boolean parameter in the TileEntity instead of asking every tick a function that checks if tesla is enabled? The need to reload the world anyway to make stuff like that work.
19: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/powerSystem/TilePowerAcceptor.java#L322
Since i know how much code is behind this 3 times in a row a getStackInSlot calls is more laggy then make a field with ItemStack which the stack in slot is adding to and then use the ItemStack given to check everything and then you do not need 3 if parameters since you can check it all in 1 go.
20: https://github.com/TechReborn/RebornCore/blob/1.11/src/main/java/reborncore/common/recipes/RecipeCrafter.java#L306
Well whats this?...

I think this list is getting you the point what i am meaning. What i listed to you what i found by just basicly looking at the code. No backtracing of what function gets called or something like that. If i go deeper i will find even more these things are just things that cause lag. Lag that is not nessesary. And these things here are not even counting TechReborn or all other of your mods.

I am just pointing out that i am at 6-8 months now updating ic2c to 1.10.2. And there is a reason why i take my time. Because else i would do these mistakes again and again. From what i have seen @modmuss50 you have great ideas for things and i like that about you. But you do not take time to flesh things out. The json destoryer you showed me is at IC2Classic at its 4th-5th version now i rewrote it 4-5 times from scratch because i found a way to do it better. And now the json destoryer is so good that i actually save so much more memory & performance on my stuff that i do not want to go back to json because that would be a bad idea in terms of performance.
Take your time to flesh things out.
And also fix this "death of 1000 needles" issue RC has.
(Just to point out on our test server we do not have much by way of transport and the only things in 1.10.2 are either behind big mods or use your core and both are not acceptable for what i want. In my beta test pack i want ic2c to be the bottleneck not everything else. That was the reason why i skip every of your mods)

And i am sry for beeing rude to you (i told already at Reddit) but from what i saw in these like 20-30 minutes just flying over code is just not good.

commented

I know the code is a mess, im no expert programmer I learnt java by making mods. some of this is remnants of 1.7. We are slowly working on cleaning it up. Thanks for pointing this out I will take a look at them at some point.

commented

My suggestion is: Stay on 1 version where you can cleanup instead of constantly trying to be at the newest version. (You get a lot of work time out of that)
Also i did learn programming the same way.

commented

Ok after going over this I believe only a few are valid points. We are currently rewriting major parts of the code base and should get cleaned up over time.

Sorry the code isnt up to your standards.