Registerable WorldFormat api#1795
Conversation
leMaik
left a comment
There was a problem hiding this comment.
I like the overall direction and the abstractions. 👍
Just some small suggestions, maybe we can simplify some code patterns while we're at it.
200298a to
bcf8d12
Compare
|
Still need to figure out the region situation, as currently a dimension needs regions for it to be viewable on the map Additionally we could provide a simplified api (for things like schematics etc.) which implements regions, regionchangewatcher, chunkdata stuff. But I think this PR is big enough already. |
bcf8d12 to
56e05d9
Compare
|
Not sure we need to go with the virtual regions approach as discussed before #1795 (comment) The changes required to support either virtual regions, or a per-world-format impl are both extremely small Regions only escape dimensions in map view in one place, and scene in one place. |
|
As long as we can somehow select areas in worlds, that's fine. Regions might be a thing that is special to java worlds anways. Even being limited to chunks is a very technical thing. Why would a user want to be limited to loading a 16x16 blocks grid. We can figure that out later, but the whole chunk/region selection could be a UI-only thing. |
leMaik
left a comment
There was a problem hiding this comment.
Mostly minor comments on the API. Overall this refactoring is still great and it's nice to see how CubicChunks, Java edition and, in the future, bedrock support fit together.
It seems like the whole regions and chunks stuff is mostly needed for the UI (outside of java world loading, of course)?
| /** For worlds that have multiple dimensions, and fully support the map view */ | ||
| public interface WorldFormat extends Registerable { | ||
| /** | ||
| * This method will be called on every possible world directory (typically this is every directory in `.minecraft/saves`). | ||
| * |
There was a problem hiding this comment.
The javadoc here doesn't explain what this class does or what the isValid method does.
| world.currentDimension().removeChunkTopographyListener(this); | ||
| World newWorld = World.loadWorld(world.getWorldDirectory(), currentDimensionId, | ||
| World.LoggedWarnings.NORMAL); | ||
| newWorld.currentDimension().addChunkTopographyListener(this); | ||
| world.loadDimension(currentDimensionId); | ||
| world.currentDimension().addChunkTopographyListener(this); |
There was a problem hiding this comment.
Removing the listeners, loading the same dimension again, and adding the listeners could be handled internally in e.g. world.currentDimension().reload() or World.reloadCurrentDimension()
WorldMapLoader and World were tightly coupled before, so this might be out-of-scope for this PR.
| mapView.setYMax(256); | ||
| } | ||
| mapLoader.loadWorldFromDirectory(PersistentSettings.getLastWorld()); | ||
| IntIntPair heightRange = mapLoader.getWorld().currentDimension().heightRange(); |
There was a problem hiding this comment.
From an API standpoint, maybe this should be HeightRange with getYMin() and getYMax(), that way we could have javadoc and document whether the values are inclusive or exclusive (and maybe have some util methods).
| } else { | ||
| yMin.setRange(0, 256); | ||
| yMax.setRange(0, 256); | ||
| if (world != null) { |
There was a problem hiding this comment.
Should we disable the sliders if no world is loaded and/or set them to some default ranges and values?
| /** | ||
| * Region X chunk width | ||
| */ | ||
| int CHUNKS_X = 32; |
There was a problem hiding this comment.
Is this independent of the world format? If this is just a UI thing for selection, we should move it into the UI classes.
There was a problem hiding this comment.
Yes, it's used in scene's chunk loading to batch by region, and in the map view for per-region selecting
As well as being used in JavaRegion for its internal state
| import java.io.IOException; | ||
| import java.nio.file.Path; | ||
|
|
||
| /** For worlds that have multiple dimensions, and fully support the map view */ |
There was a problem hiding this comment.
Since "has no dimensions" is the same as "has one dimension" and that would be supported, I guess we could lower the restrictions. The map view could draw schematics too, so that's nothing the world format should be bothered about.
Maybe change it to `A world format that Chunky can load."
| if (providedWorlds.size() > 1) { | ||
| // Maybe allow the user to select which? | ||
| // This method is called from a variety of different popup/menu situations, is this ^ possible? | ||
| Log.warn(String.format("The directory %s has multiple valid world formats: %s", dir.getAbsolutePath(), String.join(", ", providedWorlds.keySet()))); |
There was a problem hiding this comment.
Load a random one, that'll be fun in #help 🤣
There was a problem hiding this comment.
Currently the order is based on arbitrary hashmap ordering.
Though two world formats saying the same world is compatible in both would be... interesting.
| } | ||
|
|
||
| @Override | ||
| public Set<Dimension.Identifier> availableDimensions() { |
There was a problem hiding this comment.
getAvailableDimensions and getDefaultDimension
| protected int gameMode = 0; | ||
| protected final long seed; | ||
|
|
||
| /** Timestamp for level.dat when player data was last loaded. */ |
There was a problem hiding this comment.
*Timestamp when player data was last... I don't know. Loaded? Saved?
There was a problem hiding this comment.
👍 Also this should be in JavaWorld
I mainly want comments on general structure, everything else is bikesheddable. I'll go over everything again and polish it up before review.
Lots of changes here, so far I've only looked at loading worlds from the gui, haven't looked at reloading from the previous
session
Chunk,World,Dimensionare now abstract classes, the previous implementation is nowJavaChunketc.there is also a skeleton of registerable
WorldFormats. Each is asked if a directory is valid for it, if the user selects it, the format is asked to load the world.The ultimate goal would probably be to have
Chunkbe an implementation detail of java world, but that's a much larger task as it would require a rewrite of parts of the map view and scene chunk loading