-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Added Litematica support #2544
Added Litematica support #2544
Conversation
public static long roundUp(long number, long interval) | ||
{ | ||
if (interval == 0) | ||
{ | ||
return 0; | ||
} | ||
else if (number == 0) | ||
{ | ||
return interval; | ||
} | ||
else | ||
{ | ||
if (number < 0) | ||
{ | ||
interval *= -1; | ||
} | ||
|
||
long i = number % interval; | ||
return i == 0 ? number : number + interval - i; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static long roundUp(long number, long interval) | |
{ | |
if (interval == 0) | |
{ | |
return 0; | |
} | |
else if (number == 0) | |
{ | |
return interval; | |
} | |
else | |
{ | |
if (number < 0) | |
{ | |
interval *= -1; | |
} | |
long i = number % interval; | |
return i == 0 ? number : number + interval - i; | |
} | |
} | |
public static long roundUp(double number, long interval) | |
{ | |
if (number < 0){ | |
interval *= -1; | |
} | |
return interval * (long) Math.ceil(number/interval); | |
} |
I believe that this has exactly the same functionality, if you have any reason for wanting to use this instead please do, however I think this is nicer,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it will throw an exception instead of returning 0 if interval is 0 and if number is 0 it returns 0 instead of number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe that interval is ever 0, the only occurrence I could found was that it was 64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can test it. The nested class there is straight out of litematica, so I just left the code as-is.
// In 1.12, the long array isn't exposed by the libraries so parsing has to be done manually | ||
String rawBlockString = (nbt.getCompoundTag("Regions").getCompoundTag(regionName)).getTag("BlockStates").toString(); | ||
rawBlockString = rawBlockString.substring(3,rawBlockString.length()-1); | ||
String[] rawBlockArrayString = rawBlockString.split(","); | ||
long[] rawBlockData = new long[rawBlockArrayString.length]; | ||
for (int i = 0; i < rawBlockArrayString.length; i++) { | ||
rawBlockData[i] = Long.parseLong(rawBlockArrayString[i].substring(0,rawBlockArrayString[i].length()-1)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I hate parsing strings that aren't meant to be parsed, may I suggest using reflection or mixin to access the data
attribute? I checked the litematica and malilib sources and the latter is what is used there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes so that's the main reason why I have this as a draft pull because that iteration is so tacky. I'll look into those sources and try to find something.
public final class LitematicaSchematic extends StaticSchematic { | ||
|
||
public LitematicaSchematic(NBTTagCompound nbt) { | ||
String regionName = (String) nbt.getCompoundTag("Regions").getKeySet().toArray()[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not iterate over all regions?
They are all part of the schematic and the issues would just be spammed with "Baritone not loading some litematic regions"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I didn't do it because it involves reading the entire schematic into a larger volume. It makes more sense to add a region argument to the command? Going to add it to my initial issues list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to just keep the sub-regions, just like CompositeSchematic
does it with it's sub-schematics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll look into that one too.
@EmersonDove are you still working on this? |
@ZacSharp I've been on and off about it. I haven't fixed any the original bugs enough to commit. I talked to masa and he believes it should be possible but in general the 1.12.2 port has had a lot of issues. |
I'd like to express my interest here as well -- I have a lot of complex redstone that I need to place (running experimental 1.17.1 build) and this would make life much easier for me |
I ported this up to 1.17 a while ago here, did the work translating mappings etc Unsure what needs to be done to push things forward |
I't probably useable as is (never managed to get myself to have a look at it), but not ready for merging. The reason for the first point is unknown (as far as I know), the other two points have already been addressed in the discussion here. |
Hey everyone looks like I should follow up here - I wouldn't feel good merging it in with its current state - I would expect this will create GH issues than are closed. I'm interested in pushing things forward for a 1.12 implementation with @sudofox but hit a wall with the mixins and have been working on other projects (@blankly-finance). I talked to Mesa a few months ago and there is no technical reason why this couldn't work. If someone could tackle or give some pointers on the mixin for the |
For the mixin you probably want a simple mixin on |
Thanks for the mention Emerson. I should establish that my Java skills are quite hodgepodgey so I'll try to contribute as much as I can but otherwise I'll cheer you on from the sidelines 😅 |
|
||
public LitematicaSchematic(NBTTagCompound nbt) { | ||
String regionName = (String) nbt.getCompoundTag("Regions").getKeySet().toArray()[0]; | ||
this.x = Math.abs(nbt.getCompoundTag("Regions").getCompoundTag(regionName).getCompoundTag("Size").getInteger("x")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The region size needs to be handled differently. The stored sizes per axis can be negative, if the "primary corner" (region origin) is not in the minimum corner of the region. (If they are negative, then they are at max -2, there should be no -1 values created by Litematica, as that is essentially the same as positive 1.)
I think for Baritone you can basically just use Math.abs()
and then offset the origin if the values were negative for some axes. (No idea how you handle the positioning, and if you have rotations, but if you read everything into one large block container then it doesn't matter at this level anyway.) In Litematica this "freedom" in creating the selection with the primary corner in any relative corner causes quite some mess and complexity when handling the regions with rotations and mirroring etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always use the minimum corner for the position and don't have rotations, so (once we read more than the first region) just adding size + 1
and then removing the sign from size
should work fine.
I've had a look at it and I think you can copy the changes from ZacSharp@0550fe3 as is. I also had a try at loading all regions of the schematic in ZacSharp@3cf9e2f but that commit is not useable as is and comes with debug printouts and will likely (didn't try it) crash with map art mode enabled for any litematic that does completely fill its enclosing box volume. |
I've invested some more time and used the approach from #3317 (patching the holes with air) to fix |
I also failed to reproduce the bug with iron bars not being loaded correctly, all 16 worked. |
What does this PR still need to get released? |
News on why there is still no merge ? |
It's a draft? it isn't finished yet if you couldn't tell |
What's currently missing (both done on my branch but @EmersonDove hasn't done it here yet)
@leijurv I'd really like to know whether static schematics are allowed to have holes (i.e. behavior of getDirect is undefined at positions within the schematic bounds as it is the case for other schematics). |
Superseded by #3672 |
Added litematica support to 1.12.2 master.
Issues with this version:
blockstates
) isn't exposed by the libraries so parsing has to be done manually#2338 Is a fully working implementation for modern versions of MC
If anyone has knowledge or cleaner solutions for any of these leave a comment so I can update it.