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

Added biome color cache support for custom color resolver #2222

Merged
merged 3 commits into from
Jan 15, 2024
Merged

Added biome color cache support for custom color resolver #2222

merged 3 commits into from
Jan 15, 2024
+50 −42

Conversation

CPTProgrammer
Copy link
Contributor

Added biome color cache support for custom color resolver.

WorldSlice#getColor can now be invoked with a custom ColorResolver.

@jellysquid3
Copy link
Member

I don't think the solution is to duplicate code paths for "custom" and "vanilla" color resolvers, since it's fundamentally the same interface being used. There should just be one code path, and a ColorResolver->int[] map.

@CPTProgrammer
Copy link
Contributor Author

Sorry for my carelessness, I'll change the code as soon as possible

@CPTProgrammer
Update BiomeColorCache.java
eeadd45 
Merge "custom" and "vanilla" color resolvers.

Fixed issue where custom resolvers would always use the default grass color.
jellysquid3
jellysquid3 requested changes Jan 10, 2024
View reviewed changes
Copy link
Member

@jellysquid3 jellysquid3 left a comment

Choose a reason for hiding this comment

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

The abstract of this looks good, just some performance concerns.

src/main/java/me/jellysquid/mods/sodium/client/world/biome/BiomeColorCache.java Outdated Show resolved Hide resolved
src/main/java/me/jellysquid/mods/sodium/client/world/biome/BiomeColorCache.java Outdated Show resolved Hide resolved
src/main/java/me/jellysquid/mods/sodium/client/world/biome/BiomeColorCache.java Outdated Show resolved Hide resolved
src/main/java/me/jellysquid/mods/sodium/client/world/biome/BiomeColorCache.java Outdated Show resolved Hide resolved
CPTProgrammer
CPTProgrammer commented Jan 11, 2024
View reviewed changes
Copy link
Contributor Author

@CPTProgrammer CPTProgrammer left a comment

Choose a reason for hiding this comment

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

Performance optimization

src/main/java/me/jellysquid/mods/sodium/client/world/biome/BiomeColorCache.java Outdated Show resolved Hide resolved
@CPTProgrammer
Performance optimization for BiomeColorCache
3ac1355 
- Utilize Reference2ReferenceOpenHashMap: Replaced the usage of `HashMap` with `Reference2ReferenceOpenHashMap`.
- Slice Initialization Refactoring: Extracted the slice initialization branch into a new function called `initializeSlices`.
- Enhanced Populated Status Check: Implemented an incremental flag `populateStamp` (without loops) to check for populated status, replacing the reliance on two nested for-loops.
@CPTProgrammer
Copy link
Contributor Author

All requested changes have been implemented

@jellysquid3
Copy link
Member

Thanks for the contribution. It is worth mentioning that Fabric API is considering implementing an interface for mods to use custom ColorResolvers (since vanilla currently doesn't allow this, and Sodium is exposing additional functionality) but this should not affect your patch at all.

embeddedt
embeddedt reviewed Jan 13, 2024
View reviewed changes
Copy link
Contributor

@embeddedt embeddedt 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 from my end as well.

@MehVahdJukaar MehVahdJukaar mentioned this pull request Jan 15, 2024
Closed
@MehVahdJukaar
Copy link

MehVahdJukaar commented Jan 15, 2024 •
edited
Loading

Will this be added to previous version aswell?
The fact that its not possible to add custom ColorResolvers is a deal breakers for many mods that use those things and its made way worse by how locked and up unforgiving the current architecture is with that enum that cant be extended or the fact that the game hardcrashes when a color resolved that is not vanilla shows up.
This is relevant to my mod Polytone which currently crashes with Sodium

@jellysquid3
Copy link
Member

Currently there's no back-porting policy (see #2230). But we'll likely publish future versions of Sodium 0.5.x for both MC 1.20.1 (LTS) and MC 1.20.4 (Latest) in the meanwhile.

@jellysquid3 jellysquid3 merged commit 27aece3 into CaffeineMC:dev Jan 15, 2024
1 check passed
@embeddedt embeddedt mentioned this pull request Jan 18, 2024
Closed
embeddedt added a commit to embeddedt/embeddium that referenced this pull request Jan 21, 2024
@embeddedt
Port CaffeineMC/sodium-fabric#2222 and remove ColorResolverCache
325f1f3
@Poopooracoocoo Poopooracoocoo mentioned this pull request Feb 10, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@embeddedt embeddedt embeddedt left review comments

@jellysquid3 jellysquid3 jellysquid3 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
4 participants
@CPTProgrammer @jellysquid3 @MehVahdJukaar @embeddedt