Add an atlas registry#5390
Conversation
| /** | ||
| * Registers an atlas using a given texture and atlas id. | ||
| * | ||
| * @param textureId The id of the texture that will be generated. |
There was a problem hiding this comment.
Might make sense to give a example format for texture id.
Through maybe it would be good idea to rather than requiring to provide the textureId, having it be created out of the atlasId and then returned by this method? Could also be an additional method instead
| register(textureId, atlasId, createMipmaps, Set.of()); | ||
| } | ||
|
|
||
| public static void register(Identifier textureId, Identifier atlasId, boolean createMipmaps, Set<MetadataSectionType<?>> additionalMetadata) { |
There was a problem hiding this comment.
The duplicate ID checks this method performs should check against vanilla atlases as well.
| * @param atlasId The atlas id to generate a texture id for. | ||
| * @return The generated texture id. | ||
| */ | ||
| public static Identifier createTextureLocation(Identifier atlasId) { |
There was a problem hiding this comment.
Initially I thought this was used to get the texture ID of the given registered atlas, but looking at the testmod revealed that it is used to create the default texture ID for some new atlas. I would rename the method to createDefaultTextureLocation and clarify it in the javadoc.
There was a problem hiding this comment.
I hoped the create part of the name would be clear enough. I guess I can change it?
|
|
||
| @Mixin(AtlasManager.class) | ||
| class AtlasManagerMixin { | ||
| @ModifyExpressionValue(method = "<init>", at = @At(value = "FIELD", target = "Lnet/minecraft/client/resources/model/sprite/AtlasManager;KNOWN_ATLASES:Ljava/util/List;", opcode = Opcodes.GETSTATIC)) |
There was a problem hiding this comment.
I don't like how the actual value of KNOWN_ATLASES is not modified. Doing it on classload would be ideal for covering edge cases but is problematic in other ways (making the timing of the registry freeze unreliable and making checking against vanilla atlases in the registry impossible), so an acceptable way to handle this would be to still inject into <init>, but at head and modify the field instead of only its accesses in the constructor.
There was a problem hiding this comment.
I feel like all the possible injections have some weird caveat here. My ideas was that KNOWN_ATLASES doesn't necessarily need to contain all of the atlases.
A mod shouldn't really read from here anyway, everything will be available through the atlas manager itself. And even if we write our entries there, it'll be so late that a mod might as well just read from the atlas manager.
| * @param hasMipmaps Whether to generate mipmaps for the atlas. | ||
| * @param additionalMetadata Additional metadata to be set in .mcmeta files. | ||
| */ | ||
| public static void register(Identifier textureId, Identifier atlasId, boolean hasMipmaps, Set<MetadataSectionType<?>> additionalMetadata) { |
There was a problem hiding this comment.
Is there any reason for this to not accept an AtlasConfig directly?
There was a problem hiding this comment.
It allows the params to be documented, and might be a bit more stable in the case that vanilla changes the signature of the Atlas config.
But yeah you're right, this should probably just take a config directy.
3 participants
Adds a registry for adding custom atlases, and fixes the testmod double sprite source.