Miner Rework Repost#185
Merged
TechLord22 merged 26 commits intoGregTechCEu:masterfrom Nov 16, 2021
Synthitic:portingMinersCEu
Merged
Miner Rework Repost#185TechLord22 merged 26 commits intoGregTechCEu:masterfrom Synthitic:portingMinersCEu
TechLord22 merged 26 commits intoGregTechCEu:masterfrom
Synthitic:portingMinersCEu
Conversation
Member
|
With my commit - numerous improvements have been made.
|
bruberu
approved these changes
Oct 15, 2021
Member
bruberu
left a comment
bruberu
left a comment
There was a problem hiding this comment.
I think this particular PR is pretty good, and this is probably ready to go, for the most part. I don't entirely understand why the algorithm is implemented the way it is, but this should be fine. I especially like that the miner enumeration is removed.
bruberu
approved these changes
Oct 16, 2021
Member
bruberu
left a comment
bruberu
left a comment
There was a problem hiding this comment.
Soft hammering works fine on my end.
Yefancy
approved these changes
Oct 19, 2021
ALongStringOfNumbers
requested changes
Oct 26, 2021
future merge conflict is with braggest's hammering pr. other fixes are for the large miner not being able to overclock to max, adjusting tps guard numbers, drilling fluid consumption while overclocked.
htmlcsjs
reviewed
Nov 2, 2021
htmlcsjs
approved these changes
Nov 7, 2021
Member
htmlcsjs
left a comment
htmlcsjs
left a comment
There was a problem hiding this comment.
looks good, jei multiblock now works
Comment on lines
+621
to
+627
| @Override | ||
| public <T> T getCapability(Capability<T> capability, EnumFacing side) { | ||
| if (capability == GregtechTileCapabilities.CAPABILITY_CONTROLLABLE) { | ||
| return GregtechTileCapabilities.CAPABILITY_CONTROLLABLE.cast(this); | ||
| } | ||
| return super.getCapability(capability, side); | ||
| } |
Contributor
There was a problem hiding this comment.
This is not needed. There is a check for the controllable capability inside the super.getCapability() call
Member
There was a problem hiding this comment.
The check in the super method is for CAPABILITY_MULTIBLOCK_CONTROLLER, which is different from CAPABILITY_CONTROLLABLE.
Contributor
There was a problem hiding this comment.
Its in MetaTileEntity, so two classes up
Comment on lines
+138
to
+139
| this.inputFluidInventory = new FluidTankList(false, getAbilities(MultiblockAbility.IMPORT_FLUIDS)); | ||
| this.outputInventory = new ItemHandlerList(getAbilities(MultiblockAbility.EXPORT_ITEMS)); |
Contributor
There was a problem hiding this comment.
Are there notifiable versions of these that should be used? Maybe Trousers knows something
Member
There was a problem hiding this comment.
I don't know of any notifiable ItemHandler or FluidTank List classes personally.
ALongStringOfNumbers
approved these changes
Nov 16, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What:
This is identical to the previous PR but with Tech22's improvements since the previous PR went boom for whatever reason.
Some changes may not be detailed as they were in the comments of the previous PR. This PR also now points to master rather than the porting miner branch.
Attempts to mimic 5u miners except for mining pipes. Revamps scanning and breaking of blocks.
How solved:
Scan for blocks upon the machine receiving power, use lowest time complexity for sequential read and write that has dynamic size while sacrificing a bit of space complexity (linked list). Change scanning to scan for blocks instead of chunks then blocks. Blocks are broken under the miner per Y level and a pipe is rendered. A screwdriver can change what the radius is for the miner. The large miners have a chunk mode and silk touch mode toggle button in the machine UI. Chunk mode for the large miner means that instead of the origin being that of where the controller is, the origin is the center of the chunk the controller is in.
Overclocks for large miners are implemented like this.
Small miners no longer require drilling fluid.
Outcome:
Close to 5u miners. No longer have to waste resources on scanning for blocks.
Possible compatibility issue:
Maintenance will need to be integrated but there is a comment for implementing maintenance. Soft hammer pausing machine work also needs to be implemented. Toggle buttons in the large miner UI need textures. There are probably still a good amount of bugs to fix and optimizations to make. There are some debug values that show up in the machine UI that can be cleaned up at the end of the PR.