Skip to content
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

[Incomplete] Friendly Biome IDs #161

Closed
wants to merge 5 commits into from
Closed

[Incomplete] Friendly Biome IDs #161

wants to merge 5 commits into from

Conversation

justinrusso
Copy link

For #159

This adds support for referencing a biome using its resource location string, rather than the numerical id of the biome. Using id caused issues when adding a new mod that adds biomes, as IDs would be different between an existing world and a new world (just like the issues with item ids).

I've kept support in for using an id, but when the file is ever saved, it will over-write them with the resource location.

Still allow the use of integers as well, but if it cannot be parsed, assume its a resource location and try to obtain the ID from it.
Will need testing to see if this logic works OK.
biomeResourceLocation = new ResourceLocation(key);
}

Biome biome = Biome.REGISTRY.getObject(biomeResourceLocation);
Copy link
Contributor

Choose a reason for hiding this comment

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

Biome biome = ForgeRegistries.BIOMES.getValue(new ResourceLocation(key));

Use the Forge provided classes rather than the Vanilla ones.

Everything else above this is unnecessary.

@@ -76,7 +110,8 @@ protected void saveData(JsonObject json, BiomeTextureMap data) {
// Only save biomes 0-256 in this config.
// The rest goes into ExtTileTextureConfig
if (biomeID >= 0 && biomeID < 256) {
json.addProperty(String.valueOf(biomeID), data.textureMap.get(biomeID).name);
String key = Biome.REGISTRY.getObjectById(biomeID).getRegistryName().toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

String key = ((ForgeRegistry<Biome>) ForgeRegistries.BIOMES).getValue(biomeID);

Use the Forge provided classes rather than Vanilla ones.

@justinrusso
Copy link
Author

justinrusso commented Jan 18, 2018

Just realizing this does not fix the issue yet, but at least makes the config easier.

Since the biome texture config is loaded at preInit and then used everywhere else as the ID, they wont be the same IDs for an old world compared to a new one. Will have to either use the biome in more places, or make it so when a world is loaded, the IDs are "re-generated"

When getting or setting a texture the ID is used to obtain the biome itself. This *should* be all that is needed to prevent issues with worlds that have different biome IDs.
@justinrusso justinrusso changed the title Friendly Biome IDs [Incomplete] Friendly Biome IDs Jan 18, 2018
@TehNut TehNut mentioned this pull request Feb 19, 2018
@justinrusso
Copy link
Author

I feel as though this has gotten too complex for my knowledge. The real fix (at least to my understanding) would be to essentially pass around the Biome, or something of that sort rather than the ID which all methods currently accepts. This means revamping the data sent as well, which is where I started to really struggle to progress further with.

I havent worked on this in a while, so I dont recall the current state, but even just allowing the json to be the resource location rather than the int id would be of use (which is what I've gotten done so far)

@TehNut
Copy link
Contributor

TehNut commented Mar 17, 2018

I can take a look in the next few days if @Kenkron doesn't get to it first.

@TehNut
Copy link
Contributor

TehNut commented Mar 18, 2018

Looking into it a bit more, the best move would be to switch from using the numeric ID everywhere to the registry name. That, however, would require a version bump on the biome_textures file, which would break all existing configs. If that's not wanted for 1.12, it should definitely be done for 1.13.

@justinrusso
Copy link
Author

Is there any way this can be considered for 1.12? I've really enjoyed this mod as there's really nothing else like it and is certainly different than the others, but it's causing some huge headaches when trying to develop a pack with the mod in it. It results in never being able to add a new biome to the pack for example.

tyra314 pushed a commit that referenced this pull request Jan 13, 2019
This pull request includes the following major changes:

  - Allow this mod to work with JEID.
  - BiomeTextureMap and BiomeTextureConfig changed to exclusively work with Biome and Resource Location rather than IDs. This allows modded biomes to be added and removed without old maps becoming broken. *ATTENTION* This means we have a new version for the biome_textures.json with resource locations instead of biome ids.
  - Modded swamp biomes are now better supported.
  - Plains type modded biomes are no longer miscategorized as deserts.

There are other changes, like an updated MCP mapping to the latest stable version for 1.12.2.

This fixes #159 and fixes #201.
This also supersedes #161, which just no one came around to review and merge before this.
@tyra314
Copy link
Member

tyra314 commented Jan 13, 2019

I'm sorry this wasn't processed earlier, but with the merge of #207 the changes of this PR are merged already. We still like to thank you for your efforts.

Maybe that the feature is nonetheless now included in the mod is a consolation for you :(

@tyra314 tyra314 closed this Jan 13, 2019
@justinrusso justinrusso deleted the feature/1.12/friendly-biome-ids branch March 13, 2019 17:12
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.

3 participants