Skip to content

Comments

Refactor world generation#39

Merged
Cardinalstars merged 24 commits intomasterfrom
improve-worldgen
Feb 3, 2026
Merged

Refactor world generation#39
Cardinalstars merged 24 commits intomasterfrom
improve-worldgen

Conversation

@RecursivePineapple
Copy link
Collaborator

  • Work on worlgen framework some more
  • CoordinatePacker mixin
  • Prevent LightingManager NPE
  • Cleanup
  • Angelica interop
  • Lots of changes
  • spotless

Co-authored-by: Ryan Nasers <42074409+Cardinalstars@users.noreply.github.com>
TaskContainer<TTask, TResult> container = new TaskContainer<>(executor);
container.tasks.add(future);

TASK_QUEUE.addLast(container);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
TASK_QUEUE.addLast(container);
TASK_QUEUE.add(container);

It's a nitpick, but this is a bit more clear imo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer addLast because you can add to the front or to the end of the queue, and add isn't explicit enough IMO.

@github-actions
Copy link
Contributor

#40

Co-authored-by: GitHub GTNH Actions <>
Copy link
Owner

Choose a reason for hiding this comment

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

Is this something to eventually go into gtnhlib itself? Mixin on mods we control is sus obviously.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, this is the best solution. It's ugly, but we can't add any indirection to these methods or it'll kill their performance. As it is, the JVM will inline and optimize everything as if the gtnhlib versions didn't exist.

Copy link
Owner

Choose a reason for hiding this comment

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

Just delete the method if we comment it out? Or is this something wip?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I commented it out because that method didn't exist in the newer version of regionlib, and I couldn't tell what it was supposed to do. I'll have to look at it again, I haven't looked into this much further.

Comment on lines +170 to +182
@Inject(method = "getChunkSaveLocation", at = @At("HEAD"), cancellable = true, remap = false)
private void redirectChunkSaveLocation(CallbackInfoReturnable<File> cir) {
if (this.theChunkProviderServer == null) {
WorldFormatSavedData format = WorldFormatSavedData.get((WorldServer) (Object) this);

// noinspection DataFlowIssue
cir.setReturnValue(
format.getFormat()
.getWorldSaveDirectory(this.saveHandler, (WorldServer) (Object) this)
.toFile());
}
}

Copy link
Owner

Choose a reason for hiding this comment

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

What's the context on this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This decouples the world directory location from the anvil save format, since it wasn't being used at all. I forget why I had to do this, I think that world savedata wasn't being put in the right directories or something.

Copy link
Owner

Choose a reason for hiding this comment

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

I just realized one of these is named _HeightLimit and the other _HeightLimits.
Maybe make them the same name.

return delegate;
}

public static void setDelegate(IAngelicaDelegate delegate) {
Copy link
Owner

Choose a reason for hiding this comment

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

I currently see no uses of this. How is this supposed to be used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It gets called in Angelica, then CC will call the various methods on the delegate to tell Angelica when cubes and columns are loaded. I haven't put up that PR yet because there are still some cube render culling issues.

}

return new PacketHeightMapUpdate(pos, updates, heights);
return new PacketHeightMapUpdate(pos, updates.clone(), heights);
Copy link
Owner

Choose a reason for hiding this comment

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

I can sort of guess why you have to clone here, but I can't say I understand the full picture. Could you explain a bit? Is there some threading issue here that causes you needing to clone this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think there was an explicit bug, but I copied it just to be sure everything worked fine. The packet can stick around for any amount of time, but the provided updates list can be modified in the mean time.


MinecraftForge.EVENT_BUS.post(event);

tag = event.tag;
Copy link
Owner

Choose a reason for hiding this comment

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

Why reassign here?

Copy link
Owner

Choose a reason for hiding this comment

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

Wait, the event can change the tag right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, event handlers can do anything they want with the tag. It probably isn't used like that, but I did this just to make sure.

Comment on lines 359 to 367
TaskPool.submit(columnLoadExecutor, pos, tag -> {
if (tag == null) {
if (preloadFailures != null) preloadFailures.onColumnPreloadFailed(pos);
} else {
synchronized (columnCache) {
columnCache.put(pos, new SaveData(tag.orElse(null), null, System.currentTimeMillis()));
}
}
});
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't you be checking tag.isEmpty, and otherwise you don't need to do the orElse(null)?

Inspection of the columnLoadExecutor makes me think that this can't really ever be a null value given from tag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep you're right, I fixed it.

Comment on lines +253 to +275
synchronized (columnCache) {
columnCache.put(pos, new SaveData(tag, task, now));

if (columnCache.size() > 5000) {
int i = 0;

var iter = columnCache.object2ObjectEntrySet()
.fastIterator();

while (columnCache.size() > 5000 && i++ < 100) {
var e = iter.next();

SaveData data = e.getValue();

if (data.task != null && data.task.isDone()) {
data.task = null;
}

if (data.task == null && data.lastAccess + EXPIRY < now) {
iter.remove();
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

I don't really understand this part of the saveColumn. Why do this in this method? Putting in the columnCache sure, but why do this iteration here if we want to make this as fast as possible. Can't some off-thread do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll make a task + executor for this. It's meant to be a garbage collector that scans the cache when it's full to remove old SaveData objects but it's definitely not ideal now that I'm looking at it again. The i++ portion basically only allows it to scan the first 100 elements in the map, and since I'm not using the getAndMoveToLast/putAndMoveToLast methods the map ordering is undefined.

@Override
public void preloadCube(CubePos pos, CubeInitLevel wanted) {
TaskPool.submit(cubeLoadExecutor, pos, tag -> {
CubeInitLevel actual = !tag.isPresent() ? CubeInitLevel.None : IONbtReader.getCubeInitLevel(tag.get());
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
CubeInitLevel actual = !tag.isPresent() ? CubeInitLevel.None : IONbtReader.getCubeInitLevel(tag.get());
CubeInitLevel actual = tag.isEmpty() ? CubeInitLevel.None : IONbtReader.getCubeInitLevel(tag.get());

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isEmpty was added in java 11, I can't use it. Intellij kept insisting on the same change so I had to disable that lint 🤣.

Copy link
Owner

Choose a reason for hiding this comment

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

What exactly is the problem here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forced cube loading wasn't important when I wrote this PR, so to keep things simple I just disabled most of its logic. It was highly coupled to the player manager. I wanted to revisit it later once the player manager matured (which it has at this point).

Copy link
Owner

@Cardinalstars Cardinalstars left a comment

Choose a reason for hiding this comment

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

Once you look at these comments, I think it's ready. Shouldn't be much work either.

RecursivePineapple and others added 2 commits January 16, 2026 12:28
…anager.java

Co-authored-by: Ryan Nasers <42074409+Cardinalstars@users.noreply.github.com>
@Cardinalstars Cardinalstars merged commit ef4411d into master Feb 3, 2026
1 check passed
Cardinalstars added a commit that referenced this pull request Feb 4, 2026
* Work on worlgen framework some more

* CoordinatePacker mixin

* Prevent LightingManager NPE

* Cleanup

* Angelica interop

* Lots of changes

* spotless

* Works somewhat better

* Several improvements

* MORE WORLDGEN FIXES!!! + chunk api/endless ids compat + 3d biomes + other misc fixes

* Make sure it compiles

* spotless

* Fix EBS Y Level

* Checkstyle fixes

* fix dep versions

* :clueless:

* Update src/main/java/com/cardinalstar/cubicchunks/api/CCAPI.java

Co-authored-by: Ryan Nasers <42074409+Cardinalstars@users.noreply.github.com>

* Update dependency graphs

* oops

* spotlessApply (#40)

Co-authored-by: GitHub GTNH Actions <>

* Update src/main/java/com/cardinalstar/cubicchunks/server/CubicPlayerManager.java

Co-authored-by: Ryan Nasers <42074409+Cardinalstars@users.noreply.github.com>

* Review changes + biome fixes

---------

Co-authored-by: Ryan Nasers <42074409+Cardinalstars@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Cardinalstars added a commit that referenced this pull request Feb 4, 2026
* Work on worlgen framework some more

* CoordinatePacker mixin

* Prevent LightingManager NPE

* Cleanup

* Angelica interop

* Lots of changes

* spotless

* Works somewhat better

* Several improvements

* MORE WORLDGEN FIXES!!! + chunk api/endless ids compat + 3d biomes + other misc fixes

* Make sure it compiles

* spotless

* Fix EBS Y Level

* Checkstyle fixes

* fix dep versions

* :clueless:

* Update src/main/java/com/cardinalstar/cubicchunks/api/CCAPI.java

Co-authored-by: Ryan Nasers <42074409+Cardinalstars@users.noreply.github.com>

* Update dependency graphs

* oops

* spotlessApply (#40)

Co-authored-by: GitHub GTNH Actions <>

* Update src/main/java/com/cardinalstar/cubicchunks/server/CubicPlayerManager.java

Co-authored-by: Ryan Nasers <42074409+Cardinalstars@users.noreply.github.com>

* Review changes + biome fixes

---------

Co-authored-by: Ryan Nasers <42074409+Cardinalstars@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

2 participants