Skip to content

Add removing and updating modes to Structure Projector#38

Closed
ReignOfFROZE wants to merge 21 commits intoGTNewHorizons:masterfrom
ReignOfFROZE:master
Closed

Add removing and updating modes to Structure Projector#38
ReignOfFROZE wants to merge 21 commits intoGTNewHorizons:masterfrom
ReignOfFROZE:master

Conversation

@ReignOfFROZE
Copy link
Copy Markdown

@ReignOfFROZE ReignOfFROZE commented Apr 8, 2025

This is related to GTNewHorizons/GT5-Unofficial#4147. Still a WIP so leaving in draft (sprites are also going to change, those were purely just so I could tell the difference)

Also related to GTNewHorizons/StructureCompat#13

In short, this PR adds "Updating" and "Removing" modes to the Structure Projector (ItemConstructableTrigger). These modes work as follows:

  • Updating: Will update the structure of the multi to replace components that do not match the specification laid out by the projector (i.e. replacing glass, coils, blocks that aren't part of the multi but are in the way of the structure, etc). This mode will construct an entire multi from scratch if you want it to, and is really more of a "get out of my way" version of the structure projector, as it will remove anything that is in the way (except TileEntities) of the structure of the multi, similarly to how the projector works in it's current state in creative mode, but this time it actually costs something.
  • Removing: Does what it says on the tin, removes the entire multi and puts the blocks it removes into the player's inventory (or on the ground at their feet if their inventory is full). The removal logic isn't as fancy as the one from Matter Manipulator, but it will do the trick for quickly removing a multi without the use of outside tools.

The reason this PR is still in draft is because there's one slight bug with the interactions it has with an already built multi, where it will always try to replace anything that isn't a casing when in update mode, including, most notably, removing hatches on the multi (even if they're allowed to be there). I'm trying to debug it, hence all of the random System.outs and the throwables to check stacktraces of how these methods are even getting called, and if anyone has any information that can help, that would be much appreciated (more context here https://discord.com/channels/181078474394566657/603348502637969419/1358946773904785508 and here https://discord.com/channels/181078474394566657/603348502637969419/1358947157146599514). The skip logic here: https://github.com/GTNewHorizons/GT5-Unofficial/blob/58b37d4ded66fc4df4eb3aab6978a8d9bd0f2b85/src/main/java/gregtech/api/util/HatchElementBuilder.java#L504-L505
works, but for some reason it's calling both the survivalPlaceBlock method in HatchElementBuilder and the default one provided in StructureLib.

Other than that one bug (which I'm apprehensive to mark this PR as ready to merge until it's fixed, as it's sort of major considering it would be a decently annoying thing to deal with in an actual playthrough), the rest of the PR (except the sprites) is mostly ready to go and should be ready for wider testing.

@ReignOfFROZE
Copy link
Copy Markdown
Author

Bugs fixed, just need updated textures for the projector and then this should be ready to go. If someone wants to review the code itself though while those are still coming, that would be appreciated

@ReignOfFROZE ReignOfFROZE marked this pull request as ready for review April 8, 2025 15:00
@ReignOfFROZE ReignOfFROZE requested a review from a team as a code owner April 8, 2025 15:00
@Glease
Copy link
Copy Markdown
Contributor

Glease commented Apr 9, 2025

I haven't look at the code very closely. These are my feelings so far

  1. probably should move mode to AutoPlaceEnvironment, instead of a method call into a random non-api class. maybe also as an Enum instead of int
  2. delete mode should probably gets it own method in IStructureElement, with default implementation blindly calling setAir. you can see that the survivalAutoPlace methods is now largely divided into two branch: 1) place/replace 2) delete. the two code path diverges so much that there are like 13 mode < 2 checks, 8 mode == 2 checks, 2 mode == 0 checks and 5 mode == 1 checks. the disproportionally large amount of code specific to mode == 2 (21 vs 7) suggests this should really be in its own method

technical issues aside, I'd be slightly cautious towards the idea of "block removal/replacement on item use". this might have some issue with 1) chunk claims 2) early game balance of miing levels. some more testings is probably required

@Dream-Master Dream-Master added the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label Apr 9, 2025
@Dream-Master Dream-Master requested a review from a team April 9, 2025 16:19
@ReignOfFROZE
Copy link
Copy Markdown
Author

ReignOfFROZE commented Apr 9, 2025

probably should move mode to AutoPlaceEnvironment, instead of a method call into a random non-api class. maybe also as an Enum instead of int

I'm not sure what you mean by this, I'm happy to move the method but from what I can tell AutoPlaceEnvironment is for something else entirely, the getMode method is getting NBT data off the item that the method exists in, so it seemed like a good place for it. The only reason why the method is a static method on that class is to avoid needing to cast every ItemStack to an Item and then an ItemConstructableTrigger to be able to access the instance method. That aside, the enum suggestion works for me and I'll implement that.

@ReignOfFROZE
Copy link
Copy Markdown
Author

ReignOfFROZE commented Apr 9, 2025

delete mode should probably gets it own method in IStructureElement, with default implementation blindly calling setAir. you can see that the survivalAutoPlace methods is now largely divided into two branch: 1) place/replace 2) delete. the two code path diverges so much that there are like 13 mode < 2 checks, 8 mode == 2 checks, 2 mode == 0 checks and 5 mode == 1 checks. the disproportionally large amount of code specific to mode == 2 (21 vs 7) suggests this should really be in its own method

This totally makes sense, and I'll try to converge my code paths a bit as well as clean up some tech debt that I created with all of the diverging code paths to make the code easier to follow, as well as outsource some of the checking logic for deleting and updating to a helper method. I was able to make the code flow a lot better without the need for a helper method

@ReignOfFROZE
Copy link
Copy Markdown
Author

ReignOfFROZE commented Apr 9, 2025

technical issues aside, I'd be slightly cautious towards the idea of "block removal/replacement on item use". this might have some issue with 1) chunk claims 2) early game balance of miing levels. some more testings is probably required

I'll see if I can add some compat for ServerUtilities to check claims, as well as some checking to see if the block you're breaking/replacing satisfies one (or both) of these two conditions:

  1. Is a proper block that is valid for that position in the structure definition
  2. The player has an item in their inventory that is capable of mining the block they're attempting to replace (i.e. a pickaxe/shovel/wrench/wirecutter with a high enough mining level) This still seems vulnerable to exploits (such as people using a large structure with a lot of mandatory air as a quick way to remove a lot of blocks without damaging tools), as well as just hard to implement as I'd have to check for every tool type (and add a lot of compat for things such as Tinker's, etc)

If neither of those conditions is satisfied, we can probably just reject the block break/replacement.

@ReignOfFROZE
Copy link
Copy Markdown
Author

ReignOfFROZE commented Apr 10, 2025

Still working on the removal/replacement checks to make sure the blocks being removed are appropriate to remove, but the SU compat to prevent griefing and some code cleanup is there. I also need to update GTNewHorizons/GT5-Unofficial#4147 to use the enum that was added.

@ReignOfFROZE
Copy link
Copy Markdown
Author

This PR should be good to go now. Going to go update the GT5U PR now

Copy link
Copy Markdown
Contributor

@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.

(still not done with a full review yet. these are so far what I get)

I honestly won't mind if survivalPlaceBlock is a switch block calling into survivalPlaceBlockBuild survivalPlaceUpdate survivalPlaceBlockRemove. This will leads to some code duplication, but the code flow will be so much easier to understand

@serenibyss
Copy link
Copy Markdown
Member

@ReignOfFROZE Any updates on this?

@ReignOfFROZE
Copy link
Copy Markdown
Author

@ReignOfFROZE Any updates on this?

Yeah, I've been meaning to get around to it. Just haven't yet. I'll finish this up next week (or this upcoming, depending when you start your weeks) most likely.

@ReignOfFROZE
Copy link
Copy Markdown
Author

ReignOfFROZE commented Jun 3, 2025

I honestly won't mind if survivalPlaceBlock is a switch block calling into survivalPlaceBlockBuild survivalPlaceUpdate survivalPlaceBlockRemove. This will leads to some code duplication, but the code flow will be so much easier to understand

The problem with doing this is that every single implementation of IStructureElement would need to be changed, which makes this change a lot more invasive than originally intended to be.

Edit: For 2.8 purposes, I think it might be best to leave it how it is, but I would be amenable to coming back and adding this sometime during the 2.9/whatever the next release is called dev cycle and cleaning up the code flow a bit

@ReignOfFROZE ReignOfFROZE requested a review from Glease June 3, 2025 19:36
Copy link
Copy Markdown
Contributor

@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.

The code probably works as is, but there are some non-technical issues that needs addressing, such as the javadoc one

Overall I'm rather disappointed with how IStructureElement#survivalPlaceBlock now needs to take on 3 different features that is at best described as mildly related, but I guess this is what we get if we don't want a thorough refactor that takes another year to do
Removal of frames from javadoc disappoints me even more 😥

@Dream-Master
Copy link
Copy Markdown
Member

@Glease aceptable for you ?

@Dream-Master Dream-Master removed the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label Sep 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants