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 UnbakedModelDeserializer #4321

Draft
wants to merge 1 commit into
base: 1.21.4
Choose a base branch
from

Conversation

PepperCode1
Copy link
Member

@PepperCode1 PepperCode1 commented Dec 23, 2024

This pull request introduces the UnbakedModelDeserializer interface, which allows overriding the deserialization process of a JSON model file based on its fabric:type JSON key.

Points to Discuss

  • Should optional types be allowed? That is, if a type is declared as optional in the JSON file and the type does not exist, the file will be loaded as if no type were declared instead of failing to load. I am not familiar with how Fabric API load conditions work or whether they are used when deserializing models, but if they are, then they could be an alternative approach to this idea.
  • Should utilities to deserialize vanilla model elements (elements, parent, textures, ambientocclusion, display, gui_light) be provided? If so, how?
  • Should data generation classes be extended to make it easier to generate models of custom types? If so, how?
  • Which default custom types should be registered by Fabric API, if any? NeoForge provides a composite model type and an OBJ model type.
  • Should UnbakedModelDeserializer have a method to retrieve its Identifier like CustomIngredientSerializer instead of the ID being passed to the register method directly?
  • Should UnbakedModelDeserializer be generic? It represents a "type" which can create instances, and such classes are almost always generic both in Fabric API and vanilla.
  • Are there better names for certain things?

TODO

  • Documentation

@PepperCode1 PepperCode1 added the enhancement New feature or request label Dec 23, 2024
@cputnam-a11y
Copy link

cputnam-a11y commented Dec 23, 2024

Is load conditions the same thing as resource conditions? if so, https://github.com/FabricMC/fabric/blob/1.21.4/fabric-resource-conditions-api-v1/src/main/java/net/fabricmc/fabric/impl/resource/conditions/ResourceConditionsImpl.java#L66-L87 is the implentation. If a type is not specified, the resource is loaded as normal, as if all conditions passed

@Technici4n
Copy link
Member

  • optional types: yes, they are useful
  • utilities: only if we need them in FAPI for now, otherwise defer for later
  • datagen: don't think about it for now
  • custom types: can come later
  • how to pass the identifier: it does not really matter. Passing it when registering is closer to how vanilla normally registers things.
  • should the deserializer be generic? It doesn't really matter I think.


import net.fabricmc.fabric.impl.client.model.loading.UnbakedModelDeserializerRegistry;

public interface UnbakedModelDeserializer {
Copy link
Member

Choose a reason for hiding this comment

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

Missing javadoc

return UnbakedModelDeserializerRegistry.deserialize(reader);
}

UnbakedModel deserialize(JsonObject jsonObject, JsonDeserializationContext context) throws RuntimeException;
Copy link
Member

Choose a reason for hiding this comment

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

Missing javadoc, especially with regards to the context.

return UnbakedModelDeserializerRegistry.deserialize(reader);
}

UnbakedModel deserialize(JsonObject jsonObject, JsonDeserializationContext context) throws RuntimeException;
Copy link
Member

Choose a reason for hiding this comment

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

Why RuntimeException rather than a Gson one?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is acceptable for the deserializer to throw any RuntimeException, such as InvalidIdentifierException. The vanilla JsonUnbakedModel deserializer does that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants