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

Add Support for Custom ColorResolvers #3503

Merged

Conversation

PepperCode1
Copy link
Contributor

@PepperCode1 PepperCode1 commented Jan 2, 2024

Preliminary Information and The Problem

ColorResolver is a vanilla functional interface that gets a color using a biome and some positional information. Vanilla has three such ColorResolvers, namely for grass, foliage, and water, which can be found in the BiomeColors class. BlockRenderView#getColor(BlockPos, ColorResolver) is the main way to use a ColorResolver; the default implementation of this method already supports custom ColorResolvers, but it is overridden in ClientWorld. This implementation hard-codes the three vanilla ColorResolvers into an array map and invoking it with a custom ColorResolver will throw a NullPointerException and crash the game.

BlockView API v2 already provides a standardized method to retrieve the biome at a given position which can be used within BlockColorProviders. However, ClientWorld#getColor(BlockPos, ColorResolver) also provides color blending and caching; using the biome getter to recreate the functionality of a ColorResolver would either be very inefficient or very complex. Allowing the use of custom ColorResolvers would be much easier for the user.

API Design

There are two main design options for allowing custom resolvers. Either a registry is added for custom resolvers or the ClientWorld implementation is silently patched to accept custom resolvers. The former makes it explicitly known that custom resolvers are supported and makes it very difficult to pass ColorResolver objects that are only used once (such as through a capturing lambda; such resolvers are terrible for performance) while the latter is easier to use. This pull request uses the former design of using a registry and throwing an exception in ClientWorld#getColor(BlockPos, ColorResolver) if the passed ColorResolver is not registered.

Implementation

ClientWorld associates each ColorResolver to a BiomeColorCache in the hard-coded array map. The custom resolvers are not added to this map because array maps have slower retrieval times than hash maps, especially with many entries. The field type of this map is Object2ObjectArrayMap, so it is not possible to replace the field value with a hash map. Instead, custom ColorResolvers are added to a separate Reference2ReferenceOpenHashMap, which is populated once at construction time with all registered custom ColorResolvers. A ModifyExpressionValue was used to change the null cache value so that the local BiomeColorCache variable would be populated with the correct value, in case other mods use it in their own mixins.

Test Mod

The test mod was changed to add a simple block that is rendered as a solid white cube, with the top face tinted using a BlockColorProvider and custom ColorResolver. The resolver returns a magenta color in biomes with precipitation and a yellow color in biomes without. With maximum biome blending, the following effect is created.

image

@embeddedt
Copy link
Contributor

Personally I feel like forcing providers to be registered to a registry might be a better option, even if you keep the underlying map population lazy, like it is right now. Otherwise, it's too easy for a mod to silently leak huge amounts of memory by injecting many lambda instances.

@jellysquid3
Copy link
Contributor

I'm also of the opinion that requiring up-front registration is ultimately for the better here, for the reason stated, and another:

Sodium (code) will fetch all biomes and colors for an entire Y-slice of a chunk section when first accessed, and then blend the entire array of colors at once. This allows us to avoid sampling/blending hundreds of biomes every single time we need to fetch the color for a single block vertex, and is one of the reason why Sodium's chunk meshing is often 10+ times faster than Vanilla's.

This works fine when there's only a few ColorResolvers, since more likely than not, a chunk is going to have dozens of blocks using a given ColorResolver, and the overhead of sampling more upfront is quickly amortized. But if mods are allowed to register many ColorResolvers which blocks only sparsely use, this begins to create massive overhead on our side (CPU time + Memory), and will greatly slow down chunk meshing.

So I prefer any solution where mods are kind of "discouraged" from using ColorResolvers so liberally (especially in the form of inline lambdas) because it will help to avoid exploding things on our side.

Also worth mentioning: There are other mods (such as Better Biome Blend) which I believe have the same overhead problem, so this isn't a Sodium-specific thing.

@PepperCode1
Copy link
Contributor Author

The ColorResolverRegistry has now been added to avoid any issues related to performance and memory usage. Trying to use ClientWorld.getColor with an unregistered resolver will throw an UnsupportedOperationException.

@modmuss50 modmuss50 added last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge labels Feb 5, 2024
@modmuss50 modmuss50 merged commit 6fd945a into FabricMC:1.20.4 Feb 9, 2024
5 checks passed
modmuss50 pushed a commit that referenced this pull request Feb 9, 2024
* Add support for custom color resolvers

* Add ColorResolverRegistry

* Fix checkstyle

* Statically initialize all BiomeColorCaches

(cherry picked from commit 6fd945a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants