-
Notifications
You must be signed in to change notification settings - Fork 21
Use Terra-- 2.0 #49
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
Use Terra-- 2.0 #49
Conversation
Terraminusminus version: 2.0.0rc1 (channel 1.21.4) Changes: - T-- files will be stored in terraminusminus directory instead of terraplusplus - Trying to generate terrain with invalid blocks in osm.json5 will crash the server instead of silently generating unwanted terrain - Proper logging from T-- Note: version was already bumped in previous commit without taking changes into account.
2573620 to
bd85a5d
Compare
Zoriot
left a comment
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.
LGTM
Haven't tested the koppen_map, but I guess you have just recompressed it. Long Term, I think this shouldn't part of the Repo and fetched when needed via CDN, or use a similar system like heights.
One thing, it would make sense to me to keep the readmes for heights and tree_cover in the t-- repo and write it a bit more general. i think that can be used for every "plattform" & is also more coupled to t-- then t+-. Why haven't you done that?
Zoriot
left a comment
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.
Your PR introduces a new warning when building, please fix this.
Also if you want/can fix the other build warnings.
[WARNING] Unknown enum constant com.fasterxml.jackson.annotation.JsonInclude.Include.NON_EMPTY
Reason: Class file for com.fasterxml.jackson.annotation.JsonInclude$Include not found
[WARNING] Unknown enum constant com.fasterxml.jackson.annotation.JsonCreator.Mode.PROPERTIES
Reason: Class file for com.fasterxml.jackson.annotation.JsonCreator$Mode not found
[WARNING] Unknown enum constant com.fasterxml.jackson.annotation.JsonInclude.Include.NON_EMPTY
Reason: Class file for com.fasterxml.jackson.annotation.JsonInclude$Include not found
[WARNING] Unknown enum constant com.fasterxml.jackson.annotation.JsonCreator.Mode.PROPERTIES
Reason: Class file for com.fasterxml.jackson.annotation.JsonCreator$Mode not found
[WARNING] Unknown enum constant com.fasterxml.jackson.annotation.JsonInclude.Include.NON_EMPTY
Reason: Class file for com.fasterxml.jackson.annotation.JsonInclude$Include not found
[WARNING] Unknown enum constant com.fasterxml.jackson.annotation.JsonCreator.Mode.PROPERTIES
Reason: Class file for com.fasterxml.jackson.annotation.JsonCreator$Mode not found
[WARNING] Unknown enum constant com.fasterxml.jackson.annotation.JsonInclude.Include.NON_EMPTY
Reason: Class file for com.fasterxml.jackson.annotation.JsonInclude$Include not found
[WARNING] Unknown enum constant com.fasterxml.jackson.annotation.JsonCreator.Mode.PROPERTIES
Reason: Class file for com.fasterxml.jackson.annotation.JsonCreator$Mode not found
[WARNING] Unknown enum constant com.fasterxml.jackson.annotation.JsonInclude.Include.NON_EMPTY
Reason: Class file for com.fasterxml.jackson.annotation.JsonInclude$Include not found
[WARNING] Unknown enum constant com.fasterxml.jackson.annotation.JsonCreator.Mode.PROPERTIES
Reason: Class file for com.fasterxml.jackson.annotation.JsonCreator$Mode not found
[INFO] <user>/IdeaProjects/TerraPlusMinus/src/main/java/de/btegermany/terraplusminus/commands/TpllCommand.java: Some input files use or override a deprecated API.
[INFO] <user>/IdeaProjects/TerraPlusMinus/src/main/java/de/btegermany/terraplusminus/commands/TpllCommand.java: Recompile with -Xlint:deprecation for details.Using Gzip instead of XZ for the koppen dataset makes it less than 2 MiB bigger, which is negligible compared to the size of the binary, and eliminates a dependency.
Terra-- now uses the right IDs by default when correct channel is used.
This concerns both cache files and configuration files. Includes README files to help with configuration directories.
User agent follows the following pattern: "<plugin name>/<plugin version> (<terra-- lib name>/<terra-- version>; +<terra+- website)"
On Koppen map LZMA to GZIP
Pretty much. It you have a local copy of the branch, you can verify the data with the following set of commands: git checkout master
cat src/main/resources/koppen_map.lzma | unlzma | sha256sum
git checkout deps/update
cat src/main/resources/koppen_map.gz | gunzip | sha256suThe checksum should be On the custom dataset READMEs
That's a good point, but if they were to be added upstream, I would prefer it that happened in Terra++ and it would then make it's way to Terra-- and Terra+- I do however believe that Terra+- could benefit from having its own documentation that takes into account its specificity (e.g. files are not in the same places), and that would benefit from keeping the files here. What's your take on this? On the Jackson warnings
We are hitting a specific JDK bug here. The warnings should be harmless, but I have added back Jackson as an explicit dependency to get rid of them. On the TPLL deprecation warning
This is caused by the use of On other warnings
Sure. Got rid of by explicitly configuring Maven to run the Lombok annotation processor. See this JDK bug entry. |
That's fine for me, I thought T-- is a hard fork, at least it seems to be fairly far away from T++ commit wise. Also i think at some point it would make sense to make T++ a Mod which implements the T-- Library. It's fine for me to merge this in, and then when it's in T-- remove the files & adjust the paths - you should make a tracking issue then.
I agree, but that's not the README's place. The README's are in the place where the actual files have to be, so you can just refer to the current dictionary. We shouldn't refer to anything T+- specific. |
No, I am trying to keep it as close to Terra++ as possible. That still requires a lot of changes though. Last merge from upstream was a year ago and not much has happened since.
I fully agree. Or split Terra++ into a core module and implementations for different mod loaders. It doesn't really matter how at this point, but it would be infinitely better to have the generation engine at a single well defined place. The things is Terra++ development has been hanging onto BuildTheEarth/terraplusplus#29 for years now. There are so many changes in this PR it is hard to anything ambitious with the current master branch that won't come in conflict, and there are so many good changes in the PR that it would be a shame to give it up. And only @DaMatrix can really work on it at this point.
I don't really get your point here. The README's are dropped where the actual files should be, in |
Wdym with T+- specific instructions? There should'n be any specifics in there, because why do we need to have anything specific for paper? |
Configuration paths are different, and configuration options specific to Terra+- can come into conflict with native Terra-- configuration features (e.g. surface blocks, biome overrides). There is no such issue with the two datasets we are adding READMEs for, but moving them to Terra-- would risk splitting these kind of files in multiple places in the future as we support more features but with some caveats. |
Features
osm.json5, custom elevation dataset directory, custom tree coverage dataset directory)README.mdfiles are now dropped in dataset configuration folders, giving some hints on how to use themInternal changes