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

[WIP] handle multi subregion litematica schematics returning null blocks #4363

Closed
wants to merge 10 commits into from

Conversation

tolland
Copy link

@tolland tolland commented May 16, 2024

If the litematica schematic has multiple subregions that don't correspond to the full extent of the schematic, then "state" is null so the build crashes.

Copy link
Collaborator

@ZacSharp ZacSharp left a comment

Choose a reason for hiding this comment

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

placeAt only queries the block after making sure it is in bounds so a null state at this point is a bug in LitematicaSchematic.

Also logDirect should be used for direct feedback and important things like error messages, not for borderline spam which can trigger 20 times per second.

@tolland
Copy link
Author

tolland commented May 26, 2024

Background and reproduction

Try to build this litematica schematic (which has 2 subregions) using baritone 1.10.2 under minecraft 1.20.4:

subregion test1.litematic.zip

It looks like this:
2024-05-26_03 21 32

loaded schematic:
image

try and build with baritone

# litematca

It crashes with null state for block error...

[03:10:13] [Render thread/INFO]: [CHAT] [Baritone] > litematica
[03:10:13] [Render thread/INFO]: [CHAT] [Baritone] Starting layer 0
[03:10:13] [Render thread/ERROR]: Unreported exception thrown!
java.lang.NullPointerException: Cannot invoke "net.minecraft.class_2680.method_26204()" because "desired" is null
	at baritone.api.schematic.SubstituteSchematic.desiredState(SubstituteSchematic.java:51) ~[baritone-unoptimized-fabric-1.10.2.jar:?]
	at baritone.process.BuilderProcess$2.desiredState(BuilderProcess.java:475) ~[baritone-unoptimized-fabric-1.10.2.jar:?]
	at baritone.process.BuilderProcess$BuilderCalculationContext.getSchematic(BuilderProcess.java:1104) ~[baritone-unoptimized-fabric-1.10.2.jar:?]
	at baritone.process.BuilderProcess.recalcNearby(BuilderProcess.java:644) ~[baritone-unoptimized-fabric-1.10.2.jar:?]
	at baritone.process.BuilderProcess.recalc(BuilderProcess.java:620) ~[baritone-unoptimized-fabric-1.10.2.jar:?]
	at baritone.process.BuilderProcess.onTick(BuilderProcess.java:505) ~[baritone-unoptimized-fabric-1.10.2.jar:?]
	at baritone.process.BuilderProcess.onTick(BuilderProcess.java:509) ~[baritone-unoptimized-fabric-1.10.2.jar:?]
	at baritone.process.BuilderProcess.onTick(BuilderProcess.java:439) ~[baritone-unoptimized-fabric-1.10.2.jar:?]
	at baritone.utils.PathingControlManager.executeProcesses(PathingControlManager.java:199) ~[baritone-unoptimized-fabric-1.10.2.jar:?]
	at baritone.utils.PathingControlManager.preTick(PathingControlManager.java:90) ~[baritone-unoptimized-fabric-1.10.2.jar:?]
	at baritone.behavior.PathingBehavior.onTick(PathingBehavior.java:104) ~[baritone-unoptimized-fabric-1.10.2.jar:?]

@tolland
Copy link
Author

tolland commented May 26, 2024

So it was a while back when I looked at this, but the underlying issue is that baritone is returning true if the block is inside the extends of the schematic (assuming it is a rectangular cuboid) but the litematica schematic only defines the blocks for those subregions... and you can see from the image, that there are gaps between the minX and maxX etc

@ZacSharp
Copy link
Collaborator

I wasn't saying that this doesn't happen, I was saying that you are fixing the wrong file. Just like you said yourself, LitematicaSchematic.inSchematic mistakenly returns true even outside subregions. BuilderProcess relying on a bounds checked position being in bounds is fine.

@tolland
Copy link
Author

tolland commented May 26, 2024

Ah, I justed tested it... and yeah it doesn't fix the problem. So I will revisit.

@tolland tolland changed the title handle multi subregion litematica schematics returning null blocks [WIP] handle multi subregion litematica schematics returning null blocks May 26, 2024
@tolland
Copy link
Author

tolland commented May 27, 2024

Any suggestions on how to get litematica mod running under the gradle debug build in intellij? dropping litematica.jar into the mods folder is not working

Comment on lines +163 to +177
@Override
public boolean inSchematic(int x, int y, int z, BlockState currentState) {
// System.out.println("called inSchematic: " + x + "," + y + "," + z + " state: " + currentState);
for (String subReg : getRegions(nbt)) {

Vec3i offsetSubregionMin = new Vec3i(getMinOfSubregion(nbt, subReg, "x"), getMinOfSubregion(nbt, subReg, "y"), getMinOfSubregion(nbt, subReg, "z"));

if (inSubregion(nbt, subReg, x + this.offsetMinCorner.getX(), y + offsetMinCorner.getY(), z + offsetMinCorner.getZ())) {
//System.out.println("found " + x + "," + y + "," + z + " in subregion: " + subReg + " state: " + currentState);
return true;
}
}
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the default method needs a override however i have a few questions.
What is offsetSubregionMin doing? What happens if the schematic is rotated 90 degree? If im not mistaken the inSubregion() methods checks against the file in which the schematic isnt rotated.
Why not use

@Override
public boolean inSchematic(int x, int y, int z, BlockState currentState) {
    return getDirect(x, y, z) != null
}

@@ -39,7 +39,7 @@
* @author rycbar
* @since 22.09.2022
*/
public final class LitematicaSchematic extends StaticSchematic {
public class LitematicaSchematic extends StaticSchematic {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you change all the method signatures? is it for your mock test?

Copy link
Author

Choose a reason for hiding this comment

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

yeah. to mock the class to avoid having to create the BuiltInRegistries stuff

if (inSubregion(nbt, subReg, x, y, z)) {
this.states[x - (offsetMinCorner.getX() - offsetSubregion.getX())][z - (offsetMinCorner.getZ() - offsetSubregion.getZ())][y - (offsetMinCorner.getY() - offsetSubregion.getY())] = blockList[bitArray.getAt(index)];
if (inSubregion(nbt, subReg, x + this.offsetMinCorner.getX(), y + offsetMinCorner.getY(), z + offsetMinCorner.getZ())) {
this.states[x][z][y] = blockList[bitArray.getAt(index)];
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont remember why exactly i did it this way but im sure there was a reason for it.

int index = 0;
for (int y = 0; y < this.y; y++) {
for (int z = 0; z < this.z; z++) {
for (int x = 0; x < this.x; x++) {
if (inSubregion(nbt, subReg, x, y, z)) {
this.states[x - (offsetMinCorner.getX() - offsetSubregion.getX())][z - (offsetMinCorner.getZ() - offsetSubregion.getZ())][y - (offsetMinCorner.getY() - offsetSubregion.getY())] = blockList[bitArray.getAt(index)];
if (inSubregion(nbt, subReg, x + this.offsetMinCorner.getX(), y + offsetMinCorner.getY(), z + offsetMinCorner.getZ())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this counteracts the changes in the next line in some way but i dont know how

Comment on lines +187 to +212

CompoundTag region = nbt.getCompound("Regions").getCompound(subReg);

int xSize = region.getCompound("Size").getInt("x");
int ySize = region.getCompound("Size").getInt("y");
int zSize = region.getCompound("Size").getInt("z");

int xPos = region.getCompound("Position").getInt("x");
int yPos = region.getCompound("Position").getInt("y");
int zPos = region.getCompound("Position").getInt("z");

int xMin = (xSize >= 0) ? xPos : xPos + xSize + 1;
int yMin = (ySize >= 0) ? yPos : yPos + ySize + 1;
int zMin = (zSize >= 0) ? zPos : zPos + zSize + 1;

int xMax = xMin + Math.abs(xSize);
int yMax = yMin + Math.abs(ySize);
int zMax = zMin + Math.abs(zSize);

boolean withinX = xSize != 0 && x >= xMin && x < xMax;
boolean withinY = ySize != 0 && y >= yMin && y < yMax;
boolean withinZ = zSize != 0 && z >= zMin && z < zMax;

boolean found = withinX && withinY && withinZ;

return found;
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit lengthy but not wrong as far as i can tell.
the xSize != 0 check is redundant (if size is 0, xMax = xMin and x cant be >= and < to the same number)

@tolland
Copy link
Author

tolland commented May 27, 2024

Thanks for the comments! I will address them.

I opened the PR because I thought it was a simple fix, but it seems quite involved. (e.g. it seems to break rotated and mirrored multiple subregions...)

I think I will close the PR until I am more confident about how this might interact with the other stuff in LitematicaHelper

@tolland tolland closed this May 27, 2024
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.

None yet

3 participants