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

Add support for the Structure Block file format (.nbt files) #3317

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

Conversation

Aryezz
Copy link

@Aryezz Aryezz commented Mar 12, 2022

This adds support for .nbt files with the #build command. Small demo here: https://youtu.be/Gnp5JsKG9Ug

Closes #750

Notes:
Tile-Entity data doesnt get read. Schematica supports this and since this is basically copy and paste from its source, it would not be difficult to add support (but i dont know enough about Baritone to know if it would make sense).
Check out the Schematica source here: https://github.com/Lunatrius/Schematica/blob/master/src/main/java/com/github/lunatrius/schematica/world/schematic/SchematicStructure.java#L30-L43

Also note that this is the first time i've ever done anything with Java, so you might wanna check everything twice :)

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.

In my opinion this can be merged now, though I still have some thoughts/questions regarding the problem with MapArtSchematic.

If I have understood this correctly (can't test currently) minecraft:structure_void is not saved to *.nbt structure files so those contain holes where the structure void was, just like a litematic contains "holes" in areas not covered by one of its regions. If that's wrong please tell me and ignore the rest of this comment.
The current approach to patch those holes with air works without problem when the schematic is used as a normal dynamic schematic and the only use as a static schematic within Baritone is MapArtSchematic which still behaves correctly with the filled holes and I don't think there is a lot of usages by other parties, so I think we can use it. However I wonder whether static schematics are really supposed to bypass the inSchematic check. Before the attempts to read *.nbt and *.litematic files all supported formats were only capable of saving a single cuboid and MapArtSchematic can only be instantiated with schematics created by Baritone itself so when it was written it was safe to assume that no blockstate would ever be null, regardless of whether getDirect() and getColumn() are actually allowed to return null states.
My suggestion would be to explicitly allow those methods to return null for positions where inSchematic returns false (and maybe add a variant of inSchematic to IStaticSchematic that does not take the current state and returns true by default for compatibility)

@Aryezz
Copy link
Author

Aryezz commented Mar 16, 2022

If I have understood this correctly (can't test currently) minecraft:structure_void is not saved to *.nbt structure files so those contain holes where the structure void was

Yes, thats right. I guess it could be added manually, but the idea is that structure void blocks don't get saved when exporting.

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.

I have now tested this and it seems to work.

@Override
public boolean inSchematic(int x, int y, int z, IBlockState currentState) {
// Filtering out Structure Void
return (super.inSchematic(x, y, z, currentState) && this.states[x][z][y] != null);
Copy link
Member

Choose a reason for hiding this comment

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

remove outer parentheses

IBlockState[] column = this.states[x][z];
for (int i = 0; i < column.length; i++) {
// Use air blocks as placeholder for Structure Void
if (column[i] == null) { column[i] = Blocks.AIR.getDefaultState(); }
Copy link
Member

Choose a reason for hiding this comment

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

newline after { and before }


List<Template.BlockInfo> blocks = ((ITemplate) template).getBlocks();
this.states = new IBlockState[this.x][this.z][this.y];
for (Template.BlockInfo block : blocks ) {
Copy link
Member

Choose a reason for hiding this comment

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

remove the space before the )


@Override
public IBlockState[] getColumn(int x, int z) {
IBlockState[] column = this.states[x][z];
Copy link
Member

@leijurv leijurv Mar 31, 2022

Choose a reason for hiding this comment

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

This doesn't make a copy, this actually takes the states[x][z] by reference. That means that when you do column[i] = Blocks.AIR.getDefaultState(), it actually updates the value in states[x][z][i] from null to Blocks.AIR.getDefaultState(). I worry this could mess up the logic in inSchematic which checks if this.states[x][z][y] != null, as well as the logic in desiredState which also checks for null.

Copy link
Author

@Aryezz Aryezz Apr 1, 2022

Choose a reason for hiding this comment

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

I added a shallow copy in ad4f257, since it's only a 1D array and the objects just get replaced it should be enough (except if anything that uses getColumn changes the BlockStates but that would be a problem with all Schematic Formats).

@Aryezz Aryezz requested a review from leijurv September 19, 2022 08:53
Copy link
Contributor

@rycbar0 rycbar0 left a comment

Choose a reason for hiding this comment

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

This looks good to me but i dont understand why you overide this mothods in your StrucureNBT class? isnt the structure void suposed to not replace the block that is already there? i would suggest re-merg the master branch and delete the override methods. after that i think this can be merged.

@ZacSharp
Copy link
Collaborator

If you mean getDirect and getColumn swapping out null for air, that's because MapArtSchematic (and possibly third parties) assume the states are not null.
This works within Baritone because MapArtSchematic filters out the air anyway and BuilerProcess can and does use inSchematic. For third parties it can cause structure void to become air, but in my opinion that's better than crashing them.

Adding boolean inSchematic(int x, int y, int z) to IStaticSchematic (with the same default implementation as in ISchematic) would probably be a good idea though. That way, if we keep patching holes with air, old code won't crash while new code can check for the holes and exclude them from whatever it's doing.

@rycbar0 rycbar0 mentioned this pull request Nov 5, 2022
10 tasks
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.

allow #build to use .nbt files
4 participants