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.

Sign up for GitHub

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

Jump to bottom

Updates to support mod pack development #207

Merged
merged 2 commits into from
Jan 13, 2019
Merged

Updates to support mod pack development #207

merged 2 commits into from
Jan 13, 2019

Conversation

sam-kirby
Copy link
Contributor

Hi,

This pull request includes the following major changes:

  1. Allow this mod to work with JEID.
  2. 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.
  3. Modded swamp biomes are now better supported.
  4. Plains type modded biomes are no longer miscategorised as deserts.

As a result of these changes, the version number of BiomeTextureConfig has been bumped to 2.

@tyra314
Copy link
Member

tyra314 commented Jan 12, 2019

Thank you for your effort, but could you please break this down into more concise PRs?

Also, have you tested the performance impact for larger atlases for the change from ints to Biomes?

@sam-kirby
Copy link
Contributor Author

How would I go about doing performance testing?

@tyra314
Copy link
Member

tyra314 commented Jan 12, 2019

I'd start with a big map and look if there is significant lag occurring during rendering or export as png.

tyra314
tyra314 reviewed Jan 12, 2019
View reviewed changes
Copy link
Member

@tyra314 tyra314 left a comment

Choose a reason for hiding this comment

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

Actually, the patchset isn't that big. I have a few issues though.

gradle.properties Outdated Show resolved Hide resolved
gradle.properties Outdated Show resolved Hide resolved
src/main/java/hunternif/mc/atlas/client/BiomeTextureMap.java Outdated Show resolved Hide resolved
src/main/java/hunternif/mc/atlas/core/BiomeDetectorBase.java Show resolved Hide resolved
src/main/java/hunternif/mc/atlas/util/Log.java Outdated Show resolved Hide resolved
src/main/java/hunternif/mc/atlas/item/ItemAtlas.java Show resolved Hide resolved
@sam-kirby
Copy link
Contributor Author

sam-kirby commented Jan 12, 2019 •
edited
Loading

EDIT: Submitted this as your issues post appeared.
Would it be suitable to break into two pull requests?
JEID support (which includes swamp change) and change to BiomeTextureMap in a separate PR.
Plains change is a very small change, it just adds a check for whether or a biome is hot before deciding it's a desert so I'll probably just lump that in with the BiomeTextureMap PR.

It's worth noting that I started by removing reliance on the deprecated FMLLog, among other things. How should I handle this in the PRs?

Performance shouldn't be too greatly impacted by the change to Biomes. The only function that is called regularly that has been altered is BiomeTextureMap#getTextureSet. We have to do one conversion from ID to Biome per tile drawn. As the underlying map is a hash map, this should be speedy.

It's client side only, so I'll be looking for a hit to FPS rather than TPS. In all my testing so far, I have not observed this.

@sam-kirby
Copy link
Contributor Author

sam-kirby commented Jan 13, 2019 •
edited
Loading

On the subject of performance.
There seems to be no appreciable difference between performance under builds compiled from either commit 81cb2d2 or 8039f5d. Both versions perform well up to and including a map size of ~65000 chunks.
atlas_81cb2d2
Above: generated using 81cb2d2
atlas_8039f5d
Above: generated using 8039f5d

Frame rate, as well as frame time consistency as monitored using F3 debug screen, is indistinguishable between the two versions. Tested in a variety of situations, including; flying generating new chunks with atlas overlay shown and hidden, flying loading old chunks from disk, standing in various different biomes with and without rain, with and without nearby villages. Also tested was rapid panning and zooming in the GUI. This causes frame drops in both versions.

During all of these tests, average TPS remained comparable, and only going below 20TPS when generating new terrain.

Hardware used (for reference) i5-5287U laptop low power processor with Intel 6100 IGP.

Is there anything further?

tyra314
tyra314 requested changes Jan 13, 2019
View reviewed changes
Copy link
Member

@tyra314 tyra314 left a comment

Choose a reason for hiding this comment

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

I know, this is nitpicking now, but these small things add up in a large code base.

src/main/java/hunternif/mc/atlas/util/ByteUtil.java Outdated Show resolved Hide resolved
src/main/java/hunternif/mc/atlas/core/BiomeDetectorBase.java Show resolved Hide resolved
src/main/java/hunternif/mc/atlas/client/BiomeTextureMap.java Outdated Show resolved Hide resolved
src/test/java/hunternif/mc/atlas/TestNBT.java Outdated Show resolved Hide resolved
src/main/java/hunternif/mc/atlas/core/BiomeDetectorBase.java Show resolved Hide resolved
@sam-kirby
Copy link
Contributor Author

CI fails due to mapping change breaking the screenshot folder existence check. I need to rebase. Back soon :(

Sam Kirby added 2 commits January 13, 2019 10:57
Initial port to work with JEID
dbe9125 
Experiments with using resource locations internally. This probably won't stay in.

Revert "Experiments with using resource locations internally. This probably won't stay in."

This reverts commit ef27806

Possible fix - allow addition/removal of biomes after mod added to pack in old worlds.

Bug fix

Change biome categorisation so deserts are hot

Support for more than 256 biomes in biome detector. Add swamp biome type

Must do texture map load in init, after mods have registered their biomes

Match formatting

Fix test?

Handle biome removal and malformed strings in BiomeTextureConfig without crashing (whoops!)

Address issues. Fix pseudo biomes - these didn't work in a Biome keyed map.

Use LogManager directly, instead of FMLPreInitializationEvent's wrapping

Whoops, I did it again. Fixed the test.

Logger can never be null now, right?

Nits picked
Rebased and fixed broken mappings
2a753a2
@sam-kirby
Copy link
Contributor Author

Done, I think

tyra314
tyra314 approved these changes Jan 13, 2019
View reviewed changes
Copy link
Member

@tyra314 tyra314 left a comment

Choose a reason for hiding this comment

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

Looks good now, but before merging, I'd like to understand the relation of this PR to #161 and also #159.

@sam-kirby
Copy link
Contributor Author

sam-kirby commented Jan 13, 2019 •
edited
Loading

In relation to #161 I have achieved the aims of that pull request but did not go as far as TehNut suggested would be necessary.
Biomes are referred to by ResourceLocation in biome_textures.json, and the key in the associated map is the Biome object itself. Everywhere else, the mod continues to use Biome ID.
In relation to #159 it satisfies that feature request, as well as fixing the conflict described in #201

@tyra314 tyra314 merged commit a4f1ea3 into AntiqueAtlasTeam:1.12.2 Jan 13, 2019
@tyra314 tyra314 mentioned this pull request Jan 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TehNut TehNut TehNut requested changes

@tyra314 tyra314 tyra314 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sam-kirby @tyra314 @TehNut