Cover API rewrite #2071

TechLord22 · 2023-09-04T22:20:02Z

What

Rewrites the cover API for clarity and ease of maintenance. An ICoverable-like interface will no longer be the top level interface for most of MTEHolder's functionality.

Potential Compatibility Issues

There is no deprecation of the old API, so all addons will have to move to the new API when this PR is released. Existing covers in worlds will not be lost or affected in any way; this is world save backwards compatible.

ALongStringOfNumbers

What is the main difference between CoverHolder and CoverableView? It feels to me like they are sort of trying to accomplish the same thing.

src/main/java/gregtech/api/cover/CoverableView.java

src/main/java/gregtech/api/cover/Cover.java

ALongStringOfNumbers

I am unable to place covers on item pipes, specifically tested with the Large Ultimet Item Pipe and with conveyor, shutter, facade, robot arm.

As for Fluid Pipes, I can place some covers on them, however I cannot place Facade Covers on the fluid pipes.

In addition, placing a cover against a fluid pipe no longer auto connects the pipe to the cover, eg when I placed a pump against a fluid pipe, the cover was not automatically connected to the pipe. Although, using the dubug info in TOP, I think this is just rendering not getting updated for the pipe, because the open status of the pipe does change.

src/main/java/gregtech/api/cover/CoverDefinition.java

src/main/java/gregtech/common/ToolEventHandlers.java

src/main/java/gregtech/api/metatileentity/MetaTileEntity.java

ALongStringOfNumbers · 2023-09-20T16:57:02Z

src/main/java/gregtech/api/cover/CoverSaveHandler.java

+     * @param tagCompound   the tag compound to write to
+     * @param coverableView the CoverableView containing the cover
+     */
+    public static void writeCoverNBT(@NotNull NBTTagCompound tagCompound, @NotNull CoverableView coverableView) {


This is a behavior change, as previously the covers returned their modified NBT after writing to NBT, to match with MTEs. I had changed covers to return the modified NBT after write for my eventual ™️ implementation of a copy paste tool, so I would prefer if that remained here.

Can't you just store an NBT tag in a variable, pass it into the write method, and then use the tag afterwards?

ALongStringOfNumbers · 2023-09-20T17:29:32Z

src/main/java/gregtech/api/cover/Cover.java

+    default boolean canPipePassThrough() {
+        return false;
+    }


Why is this default false? I would think we want this to default true, as most covers allow pipes to pass through them?

Pipe implementations so far have had to specifically add support for specific covers. Making this return true and then not overriding to false on all new covers may cause problems with pipes. Making it "enable only if it's supported" is a better approach in my opinion.

src/main/java/gregtech/common/covers/CoverShutter.java

ALongStringOfNumbers · 2023-09-20T17:47:58Z

src/main/java/gregtech/api/items/behavior/CoverItemBehavior.java

+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Item Behavior for an Item that places a cover onto an {@link CoverHolder}.


Do we even have anything that does this?

This metaitem behavior is what all cover "containing" items use to place covers onto things. See CoverBehaviors#registerBehavior.

src/main/java/gregtech/api/cover/CoverBase.java

src/main/java/gregtech/api/cover/CoverSaveHandler.java

brachy84

This would get me a lot of merge conflicts for mui. So i would appreciate if it gets merged sooner rather than later.

src/main/java/gregtech/api/cover/CoverSaveHandler.java

ALongStringOfNumbers

After updating to the latest commits, facades now are always rendered as stone when placed on pipes and machines. After leaving the world and re-entering, they rendered correctly

When attempting to place facades onto existing quantum tanks (ie tanks that were placed before this PR), they seem like they did not place onto the machine (no rendering, no top), but after leaving the world and re-entering, they are on the quantum tank. On tanks placed after checking out this branch, they do look placed, but instead render as stone like the above.

Still having issues opening cover GUIs on pipes. This was a crafting table cover on a Large Ultimet Item Pipe to be specific.

src/main/java/gregtech/api/pipenet/block/BlockPipe.java

src/main/java/gregtech/api/pipenet/tile/TileEntityPipeBase.java

ALongStringOfNumbers · 2023-09-28T18:13:09Z

src/main/java/gregtech/api/pipenet/tile/TileEntityPipeBase.java

+        if (this.tickingPipe != null) {
+            this.tickingPipe.readFromNBT(compound);
+            return;
+        }


This data is never written to NBT, so I don't think reading it here would work

The ticking pipe is only not null when the current pipe has been replaced. That means this pipe does not exist anymore, but if the readNbt still has gone through to this, we simply redirect it to the replaced pipe.

ALongStringOfNumbers · 2023-09-28T18:16:25Z

src/main/java/gregtech/common/covers/detector/CoverDetectorBase.java

        if (tagCompound.hasKey(NBT_KEY_IS_INVERTED)) { //compatibility check
            setInverted(tagCompound.getBoolean(NBT_KEY_IS_INVERTED));
        }
+        this.redstoneSignalOutput = tagCompound.getInteger("RedstoneSignal");


Since we are adding a new NBT read here, we need to check if the NBTTagCompound has this key, because existing worlds will not have this key, so this will fail.

This NBT tag was previously defined in CoverBehavior and was just moved here. If the tag for some reason was not present, the signal produced would be zero, which should also be the default state anyway.

src/main/java/gregtech/common/items/behaviors/CoverDigitalInterfaceWirelessPlaceBehaviour.java

...gtech/common/metatileentities/multi/electric/centralmonitor/MetaTileEntityMonitorScreen.java

src/main/java/gregtech/common/pipelike/cable/BlockCable.java

src/main/java/gregtech/common/pipelike/laser/BlockLaserPipe.java

src/main/java/gregtech/common/pipelike/optical/BlockOpticalPipe.java

ALongStringOfNumbers

There seems to be a failure retrieving pipe materials now. I started in a new world, on master, placed down some Ultimet Item Pipes, attaching conveyors to a couple of the pipes. Exitted the world, loaded up in this PR, the Ultimet Item Pipes had transmuted into Aluminum Item Pipes (the default fallback material).

ALongStringOfNumbers

I can no longer use the overlay grid to adjust the connection of pipes. No matter where I am clicking on the grid, I have to be aiming at a pipe face to change connection.

Since Conveyors (and probably other item related covers) cannot attach to fluid pipes, should they still be able to bring up the overyaly grid on the fluid pipes, since interacting with the grid (or the pipe) does nothing.

…sult` on server and client

…e()` to get the correct result on the server

ghzdude · 2023-11-11T00:52:27Z

If anyone has the time, I'd like it if they could review the changes I made to fix most of the issues alson recently pointed out, but if no one can do it, that's fine.

~~Now, the only issue i can see is that newly placed covers on item pipes cannot be open their GUI before you reload the world, where the GUI is opened properly. Otherwise, the pr looks good to me.~~ I have fixed this now.

update holder to new ticking tile switch cover transfer around to be consistent with the rest of the method

Completed

Co-authored-by: brachy84 <[email protected]> Co-authored-by: Ghzdude <[email protected]> Co-authored-by: serenibyss <[email protected]>

TechLord22 added the type: refactor Suggestion to refactor a section of code label Sep 4, 2023

TechLord22 added this to the 2.8 milestone Sep 4, 2023

TechLord22 requested a review from a team as a code owner September 4, 2023 22:20

TechLord22 force-pushed the tc-cover-rewrite branch from 7f36bf9 to ca3981b Compare September 4, 2023 22:25

ALongStringOfNumbers reviewed Sep 6, 2023

View reviewed changes

src/main/java/gregtech/api/cover/CoverableView.java Outdated Show resolved Hide resolved

src/main/java/gregtech/api/cover/Cover.java Outdated Show resolved Hide resolved

ALongStringOfNumbers previously requested changes Sep 20, 2023

View reviewed changes

brachy84 reviewed Sep 22, 2023

View reviewed changes

src/main/java/gregtech/api/cover/CoverSaveHandler.java Show resolved Hide resolved

TechLord22 changed the base branch from master to 2.8 September 24, 2023 06:36

TechLord22 force-pushed the tc-cover-rewrite branch 2 times, most recently from 12e51ee to 64e746d Compare September 24, 2023 06:52

brachy84 mentioned this pull request Sep 24, 2023

Tile entity sync fixes #2091

Merged

TechLord22 force-pushed the tc-cover-rewrite branch from 64e746d to b5e3367 Compare September 24, 2023 22:14

ALongStringOfNumbers reviewed Sep 28, 2023

View reviewed changes

ALongStringOfNumbers reviewed Oct 11, 2023

View reviewed changes

ALongStringOfNumbers reviewed Oct 12, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

serenibyss force-pushed the 2.8 branch 2 times, most recently from c5721e7 to 1ccfb48 Compare October 31, 2023 02:37

TechLord22 and others added 12 commits November 4, 2023 18:59

start of rewrite

72e040e

initial cover port

1b63963

cover fixes

a43a185

remove old api

672e859

fix failing tests

8f29d55

review comments

fe0a65c

fix facades rendering as stone until render update

9dcc411

review comments and fixes

03bb781

fix ticking pipe desyncs

f79a4d5

fix facade cover desync

b13879b

clean up BlockPipe#hasPipeCollisionChangingItem and related

12d10ed

cover implementation comments

ac212be

TechLord22 and others added 7 commits November 4, 2023 19:00

remove crafting cover

7789e94

fix pipe cover raytraces

51345fc

read/write before calling super

529e371

use RayTracer::rayTraceCuboids to get the correct `CuboidRayTraceRe…

61802db

…sult` on server and client

move the fixed raytrace call to its own public method

83a7cdb

use BlockPipe::getServerCollisionRayTrace for `rayTraceCoverableSid…

304f918

…e()` to get the correct result on the server

fix rebase

54b87ad

TechLord22 force-pushed the tc-cover-rewrite branch from b997980 to 54b87ad Compare November 4, 2023 23:02

ghzdude and others added 2 commits November 10, 2023 19:47

make holder in PipeCoverableImplementation not final

f12757a

update holder to new ticking tile switch cover transfer around to be consistent with the rest of the method

allow levers, buttons, redstone dust etc on top of covers on pipes

ffe4a66

TechLord22 merged commit fe618cc into 2.8 Nov 12, 2023
2 checks passed

TechLord22 deleted the tc-cover-rewrite branch November 12, 2023 22:04

serenibyss added a commit that referenced this pull request Nov 23, 2023

Cover API rewrite (#2071)

87e9810

Co-authored-by: brachy84 <[email protected]> Co-authored-by: Ghzdude <[email protected]> Co-authored-by: serenibyss <[email protected]>

magcicada mentioned this pull request Nov 30, 2023

Mod will get crash with gtceu 2.8.0+ brachy84/MechTech#11

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cover API rewrite #2071

Cover API rewrite #2071

TechLord22 commented Sep 4, 2023

ALongStringOfNumbers left a comment

ALongStringOfNumbers left a comment

ALongStringOfNumbers Sep 20, 2023

TechLord22 Sep 21, 2023

ALongStringOfNumbers Sep 20, 2023

TechLord22 Sep 21, 2023

ALongStringOfNumbers Sep 20, 2023

TechLord22 Sep 21, 2023

brachy84 left a comment

ALongStringOfNumbers left a comment

ALongStringOfNumbers Sep 28, 2023

brachy84 Sep 28, 2023

ALongStringOfNumbers Sep 28, 2023

TechLord22 Oct 3, 2023

ALongStringOfNumbers left a comment

ALongStringOfNumbers left a comment

This comment was marked as resolved.

ghzdude commented Nov 11, 2023 •

edited

Loading

Cover API rewrite #2071

Cover API rewrite #2071

Conversation

TechLord22 commented Sep 4, 2023

What

Potential Compatibility Issues

ALongStringOfNumbers left a comment

Choose a reason for hiding this comment

ALongStringOfNumbers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brachy84 left a comment

Choose a reason for hiding this comment

ALongStringOfNumbers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ALongStringOfNumbers left a comment

Choose a reason for hiding this comment

ALongStringOfNumbers left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

ghzdude commented Nov 11, 2023 • edited Loading

ghzdude commented Nov 11, 2023 •

edited

Loading