Skip to content
This repository has been archived by the owner on Mar 8, 2024. It is now read-only.

Fix a bug in mega distill tower #395

Closed

Conversation

oier-namespace-std
Copy link

Problem: In game the incompleted mega distill tower is detected as completed.

issue

Possible reason: structure check forgot to ensure that the top layer is completed.

PS: I do not have a Java debug environment. :<

@Dream-Master Dream-Master requested a review from a team February 29, 2024 05:44
@oier-namespace-std
Copy link
Author

Bug analysis:

In file bartworks/common/tileentities/multis/mega/GT_TileEntity_MegaDistillTower.java, 293-294

        while (this.mHeight < 12 && this.checkPiece(STRUCTURE_PIECE_LAYER, 7, this.mHeight * 5, 0)
                && !this.mTopLayerFound) {

If the top layer structure check fails (the top layer has air), but checkPiece() found at least one machine casing (which will call onTopLayerFound() (line 181) and set this.mTopLayerFound to True), then we will get out of the loop and enter final check (305-307):

        return this.mCasing >= 75 * this.mHeight + 10 && this.mHeight >= 2
                && this.mTopLayerFound
                && this.mMaintenanceHatches.size() == 1;

And these checks will eventually pass, resulting in a weird 'completed structure' above.

So maybe rewritting the logic on line 293-294 (report incomplete structure instantly after checkPiece() fails) is better ig?

Copy link
Member

@AbdielKavash AbdielKavash left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation! I have tested it in the full pack, and it fixes the problem.

Can I ask for one more thing? Explicitly check whether the top "layer" contains an output hatch. The regular DT will not form if any layer doesn't have an output hatch, but the MDT allows the top layer without an output.

Copy link

@Glease Glease left a comment

Choose a reason for hiding this comment

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

this is not how it should be done.

  1. HINT piece should never be used in checkMachine()
  2. checking the same space twice for the convenience of coding is slightly too wasteful in a mega machine

this is really just a duplication of GTNewHorizons/GT5-Unofficial#865 and the correct fix is do what that PR was doing.
I'll prepare another PR in the mean time to do what I suggested.

@Glease Glease mentioned this pull request Feb 29, 2024
@oier-namespace-std
Copy link
Author

this is not how it should be done.

  1. HINT piece should never be used in checkMachine()
  2. checking the same space twice for the convenience of coding is slightly too wasteful in a mega machine

this is really just a duplication of GTNewHorizons/GT5-Unofficial#865 and the correct fix is do what that PR was doing. I'll prepare another PR in the mean time to do what I suggested.

After the bug analysis I realized that the piece check is actually done but the old code didn't deal with the returning results correctly. So I totally agree with changes in the next PR.

Thanks for review!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants