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

Merged

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.
    Resolution: Yes. The behavior and syntax of this feature matches those of NeoForge and is explained in the documentation of this pull request. Resource conditions may in theory work as a functionally equivalent alternative but would be significantly less convenient to use, requiring the creation of multiple files per model.
  • Should utilities to deserialize vanilla model elements (elements, parent, textures, ambientocclusion, display, gui_light) be provided? If so, how?
    Resolution: They will not be included in this pull request but may be added in a later one.
  • Should data generation classes be extended to make it easier to generate models of custom types? If so, how?
    Resolution: They will not be included in this pull request but may be added in a later one.
  • Which default custom types should be registered by Fabric API, if any? NeoForge provides a composite model type and an OBJ model type.
    Resolution: None will be included in this pull request but some may be added in a later one.
  • Should UnbakedModelDeserializer have a method to retrieve its Identifier like CustomIngredientSerializer instead of the ID being passed to the register method directly?
    Resolution: No; not necessary.
  • 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.
    Resolution: No; not necessary.
  • Are there better names for certain things?
    Resolution: Only naming of the single added API class, UnbakedModelDeserializer, and its members matters. These names are already good. Class and member names in the implementation can be changed at any later time.

TODO

  • Documentation
  • Optional types

@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.

@XFactHD
Copy link

XFactHD commented Jan 18, 2025

  • Optional types: yes, they are very useful if the basic model can be loaded by vanilla and the custom loader just enhances the model
  • Deserialization utilities: something like Neo's StandardModelParameters could certainly be useful but I don't think it should be a blocker for this PR
  • Datagen: I don't know to which degree Fabric extends model datagen for custom models beyond parent and textures, so I can't really comment on this
  • Custom types: better suited for subsequent PRs IMO
  • Identifier lookup: I can only see this being useful for datagen and for that it should be perfectly fine to make the registry a BiMap and look up the key from that (assuming of course that the deserializer is even involved in datagen at all)
  • Generics: I don't think there is much value in a generic type in this case

Apart from optional types and the docs TODO, the implementation looks good to me.

@PepperCode1 PepperCode1 marked this pull request as ready for review February 4, 2025 02:50
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Looks great, nice and simple API surface.

@modmuss50 modmuss50 added the last call If you care, make yourself heard right away! label Feb 4, 2025
Co-authored-by: Juuz <6596629+Juuxel@users.noreply.github.com>
@modmuss50 modmuss50 added the merge me please Pull requests that are ready to merge label Feb 7, 2025
@modmuss50 modmuss50 merged commit ae23723 into FabricMC:1.21.4 Feb 9, 2025
4 checks passed
modmuss50 pushed a commit that referenced this pull request Feb 9, 2025
* Add UnbakedModelDeserializer

* Document UnbakedModelDeserializer

* Allow custom model types to be optional

* Update javadoc as per suggestion

Co-authored-by: Juuz <6596629+Juuxel@users.noreply.github.com>

---------

Co-authored-by: Juuz <6596629+Juuxel@users.noreply.github.com>
(cherry picked from commit ae23723)
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.

6 participants