Non-NBT/MCA code cleanups #374
Conversation
…pertype This grammatical nightmare has surprisingly been approved by a colleague that teaches English.
RandomAccessFile implements DataOutput
| source = entities; | ||
| } else { | ||
| // MAINTAINER why not check poi data? | ||
| // MAINTAINER fail fast? |
There was a problem hiding this comment.
Addressed in 3738e76
| @@ -13,6 +14,7 @@ public boolean relocate(Point3i offset) { | |||
| } | |||
|
|
|||
| private boolean relocateChunk(Chunk c, Point3i offset) { | |||
| // MAINTAINER why do we check if DataVersion exists, shouldn't that be a (fast-failing) irrecoverable corruption? | |||
There was a problem hiding this comment.
In other words: why is it okay to silently ignore this chunk and continue the relocation operation if DataVersion is missing?
There was a problem hiding this comment.
That is true. It could log a warning in case of failure to find the DataVersion tag, but only if the chunk has any data (c.getData() != null), otherwise it should just ignore that chunk.
| } else if (entities != null && entities.getData() != null) { | ||
| source = entities; | ||
| } else { | ||
| // MAINTAINER why not check poi data? |
There was a problem hiding this comment.
I agree, this should also check for the DataVersion in poi just in case.
| @@ -82,11 +70,9 @@ jar { | |||
| 'Class-Path': configurations.shadow.files.stream() | |||
| .filter($it -> !$it.name.startsWith('javafx')).collect{"lib/$it.name"}.join(' ') | |||
| ) | |||
| exclude 'licenses/' | |||
| from 'LICENSE' | |||
| dependsOn minifyCss | |||
| dependsOn copyRuntimeLibs | |||
There was a problem hiding this comment.
copyRuntimeLibs needs to be adjusted too, because it still references configurations.shadow.
Those libs are needed for the *-min.jar as well as for bundling with the Windows Installer.
| @@ -21,7 +21,7 @@ protected Integer getNumber(ChunkData data) { | |||
| if (data.region() == null || data.region().getData() == null) { | |||
There was a problem hiding this comment.
This check is going to be inconsistent with getDataVersion and i think it should be removed entirely.
It could use ValidationHelper.withDefault(data::getDataVersion, 0); if we don't want it to log a warning on failure to find a DataVersion, but i think it would make more sense if it logs the warning because it would imply a chunk corruption.
I also noticed that Region#getFilteredChunks does not check if a chunk is completely empty. There should be a check like this:
if (regionChunk == null && entitiesChunk == null && poiChunk == null) {
continue;
}This should stop all filters requiring a DataVersion from throwing an exception if a chunk is completely empty because they shouldn't even be executed.
| }; | ||
| ExposedByteArrayOutputStream baos = new ExposedByteArrayOutputStream(); | ||
|
|
||
| // CHECK DataOutputStream wrapper unnecessary? |
There was a problem hiding this comment.
Both wrappers are unnecessary, because NBTWriter#write will wrap with a BufferedOutputStream and a DataOutputStream.
So this should look like this:
OutputStream nbtOut = switch (compressionType) {
case GZIP, GZIP_EXT -> new GZIPOutputStream(baos);
case ZLIB, ZLIB_EXT -> new DeflaterOutputStream(baos);
case NONE, NONE_EXT -> baos;
};There was a problem hiding this comment.
Same for both of the Chunk#load methods.
| if (sourceVersion == 0) { | ||
| continue; | ||
| } | ||
| int sourceVersion = sourceChunk.getDataVersion(); |
There was a problem hiding this comment.
This now will cancel the merging process of the entire file / all files if it throws the NoSuchElementException, as opposed to before where it simply ignored it.
There was a problem hiding this comment.
Thinking about this, we'll need to look into all other places where this might be an issue
| return level; | ||
| } | ||
|
|
||
| // XXX what's going on with the colliding fixEntityUUID* methods? What do they actually do? How can they be named better? |
There was a problem hiding this comment.
fixEntityUUIDsMerger handles entities on the root level, while fixEntityUUIDMerger is used to recursively change entity UUIDs for any passengers.
I guess they could be renamed to something like recalculateEntityUUID and recalculateEntityUUIDs to better describe what they do.
|
|
||
| // XXX what's going on with the colliding fixEntityUUID* methods? What do they actually do? How can they be named better? | ||
| public static void fixEntityUUIDsMerger(CompoundTag root) { | ||
| // MAINTAINER shouldn't this be VersionController'ed? |
There was a problem hiding this comment.
Yes, but then we need to make sure that the root passed to this function is actually the root CompoundTag, and not already the Level tag in earlier versions.
As mentioned on Discord, these are my cleanups & refactors so far that have nothing to do with NBT or MCA.
The rest of my de-duplication effort depends on the discussed moving of most NBT/MCA logic to Querz/NBT, so I'll look into that next.
I'd strongly recommend reviewing this code per-commit.
I've rebased, split and rewritten the commit history countless times in an effort to make everything as review-friendly (and easy to revert) as possible, so I hope there's no weird leftovers remaining... :)