Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miner #2170

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Miner #2170

wants to merge 34 commits into from

Conversation

Tictim
Copy link
Contributor

@Tictim Tictim commented Nov 16, 2023

What

Just a small refactor PR haha, don't worry








ok i just rewrote everything

Implementation Details

All contents of MinerLogic class was revisited and refactored. It now has:

  • Less unused properties (lol),
  • New, improved tick logic,
  • More flexible mining area API support, and more.

Among all other, the ore search algorithm is greatly improved; now the ore checking is handled by BlockUtility#isOre, with significantly less overhead compared to previous approach. Additionally, API for adding custom ore blockstate (or, excluding specific blockstate from ore) is available. I've migrated prospector widget to the new API.

All miner implementations were revisited as well, and they feature numerous major changes both code-wise and gameplay-wise.

All miner-related classes, as well as new ones, were moved to gregtech.common.metatileentities.miner. Client-sided packages are located in appropriate client packages.

All miners (both singleblock and multiblock) have new toggleable feature that visualizes miner's operating area in the world, accessible in UI. This feature was implemented with transitive flag, meaning the area preview will disappear once the miner gets unloaded. I don't know if this is a good idea. Also do NOT look into the rendering code, as the code in question is classified as [DATA EXPUNGED]

All miners have new UI, with more helpful information (hopefully) and less garbage internal values dumped in text panel.

Multiblock miners have three new features:

  • An option to adjust amount of Y layers to operate before stopping. For example, Y limit of 3 means the miner will operate 3 blocks deep before stopping.
  • An option to repeat scan on stop. Note that the behavior of miners are changed so they do not consume resources on idle scan.
  • An option to disable ore replacement. Normally, the ore block is replaced with cobblestone (or other config-specified block) unless that block is on mining pipe column. If you have the ore replacement disabled, the ore will be replaced with plain air instead.

Mining pipe has been re-implemented using entities, to provide proper collision and prevent players from just falling off the pit. The new mining pipe rendering uses customizable vanilla model system, and they properly render lighting effects now.

Also the singleblock miners have new textures.

Oh. Also the steam miner is gone. Rest in peas my bro ;((

Outcome

  • New recipe for all miners (both singleblock and multiblock).
  • Chunk mode does not have area 1 tile shorter than non-chunk mode area. (Well, it's the other way actually, non-chunk mode area was 1 tile longer than chunk mode area. Now they have identical ranges.)
  • Fixed bug that made miners void block on certain conditions, potentially resulting in removal of unbreakable/important block.

Additional Information

There's number of implementation details and behavior changes that need to be discussed before the PR gets accepted.

  • The update logic change introduces a good bit of delay to mining task. Previously the column block was deleted whenever the pipe needed to go there, and the new logic actually accounts mining pipe columns and applies delay to miner extending pipe / clearing out space for the pipe to go down. This behavior change will most likely slow down all miner's operation.
  • Miners no longer consume resources on idle scan. I don't know if this bit was a mistake, or intended design, but miners doing nothing while simultaneously eating away drilling fluid is kind of nonsense so here we go.
  • With the new changes the EV multiblock miner has less range than HV singleblock miner (48 vs 49). Idk I just find it funny
  • I just deleted entirety of screwdriver interaction, because you can adjust ranges using UI now, which is always better.

The method GTUtility#isOre was deprecated in favor of BlockUtility#isOre.

I'm planning to make standalone addon that adds back steam miner so anyone using it can keep the machine. I have few more ideas for the mod so I'll post about it on Discord or sth once it's done.

Potential Compatibility Issues

Existing words shouldn't blow up or anything, but the miner code is pretty much incompatible with previous one so most likely all configurations (maybe chunk mode/silktouch would be fine?) will reset and the miner will start over. Can't say much. Would appreciate if someone tests it.

A number of recipe and lang entry changes will need some work. Resource packs need to override new machine overlay for single miners.

If somebody says they're using miner code, they're not real. Don't trust a word.

…ssages, it's not funny, it's rather annoying and doesn't convey any meaningful information to outsiders. Just because you don't understand what you did in last 5 days doesn't mean you are exempted from your duties of explaining your works to your collaborators. Anyways, here's a small refactor.
# Conflicts:
#	build.gradle
#	src/main/java/gregtech/client/renderer/texture/Textures.java
#	src/main/java/gregtech/common/metatileentities/multi/electric/MetaTileEntityFluidDrill.java
#	src/main/java/gregtech/common/metatileentities/multi/electric/MetaTileEntityLargeMiner.java
#	src/main/resources/assets/gregtech/lang/en_us.lang
@Tictim Tictim requested a review from a team as a code owner November 16, 2023 05:25
@Tictim Tictim added type: feature New feature or request type: refactor Suggestion to refactor a section of code labels Nov 16, 2023
@@ -314,8 +316,6 @@ public static void init() {
STEAM_ROCK_BREAKER_BRONZE = registerMetaTileEntity(19, new SteamRockBreaker(gregtechId("steam_rock_breaker_bronze"), false));
STEAM_ROCK_BREAKER_STEEL = registerMetaTileEntity(20, new SteamRockBreaker(gregtechId("steam_rock_breaker_steel"), true));

STEAM_MINER = registerMetaTileEntity(21, new SteamMiner(gregtechId("steam_miner"), 320, 4, 0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you leave a comment saying that this ID is reserved for the steam miner addon, if you are going to use this id


@Override
public boolean drainMiningResources(@Nonnull MinedBlockType minedBlockType, boolean pipeExtended, boolean simulate) {
if (minedBlockType == MinedBlockType.NOTHING) return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that no energy is used in the scanning period while running a multiblock?

@@ -26,6 +26,7 @@
import gregtech.common.blocks.BlockGlassCasing;
import gregtech.common.blocks.MetaBlocks;
import gregtech.common.metatileentities.MetaTileEntities;
import gregtech.common.metatileentities.miner.MetaTileEntityLargeMiner;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥺

@serenibyss serenibyss added this to the 2.8 milestone Nov 19, 2023
# Conflicts:
#	src/main/java/gregtech/api/capability/GregtechDataCodes.java
#	src/main/java/gregtech/api/capability/impl/RecipeLogicSteam.java
#	src/main/java/gregtech/api/util/GTUtility.java
#	src/main/java/gregtech/client/ClientProxy.java
#	src/main/java/gregtech/common/metatileentities/electric/MetaTileEntityMiner.java
#	src/main/java/gregtech/common/metatileentities/multi/electric/MetaTileEntityLargeMiner.java
#	src/main/java/gregtech/common/metatileentities/steam/SteamMiner.java
#	src/main/resources/assets/gregtech/lang/en_us.lang
Copy link
Contributor

@ALongStringOfNumbers ALongStringOfNumbers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Large Miner GUI should remember if you were on the information or configuration screen when you last closed the screen.

Also, the mining preview seems to go all the way to bedrock, but it might be better just to render it on the level that the controller is on.

Can this PR deal with all the stuff brought up in #1751, if it is even still applicable?

Having some issues with the Buttons for Silk Touch Mode, Chunk Mode, and Repeat After Finished. I am only able to change the button display when clicking on the Repeat After Finished button, which toggles all three buttons. At other times, clicking on the other two buttons does nothing (in GUI at least)

Thoughts on having the final multiblock miner require the Assembly Line for its recipe?

this.isHighPressure ? 12 : 6, true,
ConfigHolder.machines.machineSounds && !this.metaTileEntity.isMuffled())) {
setNeedsVenting(false);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this setVentingStuck here still first check !ventingStuck?

long resultEnergy = energyContainer.getEnergyStored() - energyToDrain;
if (resultEnergy >= 0L && resultEnergy <= energyContainer.getEnergyCapacity()) {
if (!simulate) {
energyContainer.changeEnergy(-energyToDrain);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is energyContainer.removeEnergy btw, if you want the additional clarity at all in the method calls

state.getBlock().getDrops(drops, world, pos, state, 0); // fallback
}
boolean result = GTTransferUtils.addItemsToItemHandler(inventory, true, drops);
this.inventoryFull = result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? Just because we can add something to the inventory does not mean that the inventory will necessarily be full.

int yLimit = this.minerLogic.getYLimit();
ITextComponent value;
ITextComponent hoverText;
if (yLimit > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be limited by the height of the multiblock above bedrock, or something that just prevents it from increasing forever

* @param simulate If {@code true}, this action will not affect the state of the game
* @return Whether the action was successful
*/
boolean drainMiningResources(@Nonnull MinedBlockType minedBlockType, boolean pipeExtended, boolean simulate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The boolean for pipeExtended does not seem to be used in any of the implementations of this method. Was it left over from an old implementation?

@serenibyss serenibyss removed this from the 2.8 milestone Nov 25, 2023
# Conflicts:
#	src/main/java/gregtech/api/capability/IMiner.java
#	src/main/java/gregtech/api/capability/impl/RecipeLogicSteam.java
#	src/main/java/gregtech/api/capability/impl/miner/MinerLogic.java
#	src/main/java/gregtech/api/capability/impl/miner/MultiblockMinerLogic.java
#	src/main/java/gregtech/api/capability/impl/miner/SteamMinerLogic.java
#	src/main/java/gregtech/api/gui/GuiTextures.java
#	src/main/java/gregtech/api/util/BlockUtility.java
#	src/main/java/gregtech/api/util/GTUtility.java
#	src/main/java/gregtech/client/ClientProxy.java
#	src/main/java/gregtech/client/renderer/texture/Textures.java
#	src/main/java/gregtech/common/MetaEntities.java
#	src/main/java/gregtech/common/metatileentities/MetaTileEntities.java
#	src/main/java/gregtech/common/metatileentities/electric/MetaTileEntityMiner.java
#	src/main/java/gregtech/common/metatileentities/multi/electric/MetaTileEntityLargeMiner.java
#	src/main/java/gregtech/common/metatileentities/steam/SteamMiner.java
#	src/main/java/gregtech/common/terminal/app/prospector/widget/WidgetProspectingMap.java
#	src/main/java/gregtech/loaders/recipe/MetaTileEntityLoader.java
@Tictim Tictim requested a review from a team as a code owner December 1, 2023 02:54
MinerUtil.DISPLAY_CLICK_CHUNK_MODE_ENABLE, MinerUtil.DISPLAY_CLICK_CHUNK_MODE_DISABLE)));

textList.add(new TextComponentTranslation("gregtech.machine.miner.display.silk_touch",
toggleButton(this.minerLogic.isRepeat(), this.minerLogic.isWorking(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like all the GUI buttons are passing in this.minerLogic.isRepeat() for whether they are on or off, instead of their unique booleans.

@serenibyss serenibyss removed the request for review from a team December 7, 2023 03:34
# Conflicts:
#	src/main/java/gregtech/api/util/BlockUtility.java
# Conflicts:
#	src/main/java/gregtech/common/metatileentities/MetaTileEntities.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request type: refactor Suggestion to refactor a section of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants