[SOLVED] IHasModel alternative for more complex situations

[jaminv]

I followed a tutorial sometime ago, and of course I ended up with an IHasModel interface buried in my code. At the time, it kind of made sense because it wasn't really as simple as "of course everything has a model". Some blocks actually had custom item variants, so they would provide a different registerModels() implementation that handled the custom item variants. At the time, it felt like a pretty good use of abstraction. Except that in order to avoid a bunch of duplicate code, every Block derives from a `BlockBase` class that provides little more than a default registerModels() implementation... which is definitely *not* a good use of inheritance.

 

Now, my situation has become a great deal more complex. I have some blocks that have custom state mappers for customer model baking. Currently, these utilize a `BakedModelProvider` interface because the block needs to provide data to the BakedModelLoader. I'm not sure if this is just a `IHasModel` by another name. There's no actual `registerModels()` method to the interface. Because they are all handled the same, I've put the following code in my ModelRegistryEvent:

 

if (block instanceof BakedModelProvider) {
	ModelLoader.setCustomStateMapper(block, new CustomStateMapper(new ModelResourceLocation(block.getRegistryName(), "normal")));
	BakedModelLoader.register(block.getRegistryName().getResourcePath());				
}

 

Naturally, things have gotten even more complex than this as my default `CustomStateMapper` implementation isn't going to work for every model, so I may actually need to add a `registeryModels() ` method to my `BakedModelProvider` interface.

 

---

 

I'm not sure exactly what about the IHasModel makes it an anti-pattern. From my experience, it seems that the forced inheritance (or duplicate code) required to make the abstraction work is the main culprit. If that's the case, then an `IHasCustomModel` class would still be a proper pattern, whereas an `IHasModel` would not. Is that so? Or is there something else that makes it an anti-pattern?

Edited August 27, 2019 by jaminv

[Draco18s]

1) You don't need IHasModel at all. All items need models. ALL OF THEM. ALL OF THEM NO EXCEPTIONS and the data required to register one is public. Blocks get their models registered automatically (this is true of items as well come 1.13). Its an antipattern because you will never have an item that does not have a model, so utilizing an interface to supply it is worse than useless (your blocks that require IHasModel? They have ItemBlocks and ItemBlocks are items and require a model). You're actually writing more code than not using the interface.

 

2) block instanceof BakedModelProvider is bad. BakedModelProvider is almost certainly client side only (or references something that's client side only) and will crash the dedicated server

 

 

Edited August 25, 2019 by Draco18s

[jaminv]

1) Some models require additional consideration. I'm asking if IHasCustomModel is an acceptable pattern when a block requires extra code to handle its models. The data required to build the models is indeed public, but it may need special information like a Enum specific to that block. It makes more sense (to me) to localize that code in the same package as the block itself rather than push it into the registry packages.

 

2) BakedModelProvider is only referenced in my ModelRegistryEvent (and there only by instanceof), and in my BakedModelLoader, which is only registered in the ClientProxy. Additional, the only method of interface is specified as SideOnly (Side.CLIENT).  I've actually modelled it after CodeChickenLib's IBakeryProvider (except my implementation is vastly simpler). Not saying that just because someone else did it makes it correct, but it seems like a lot of consideration has been applied here.

(also the only method does nothing more than return an object, the `bakeModel` method has to be run on that object before any client-side code is referenced, and that is only done after BakedModelLoader provides a model to the client and bake() is run on that model).

 

It appears that the ModelRegistryEvent is only ever run on the client side. Is that correct?

Edited August 25, 2019 by jaminv

[Draco18s]

1 hour ago, jaminv said:

 

It appears that the ModelRegistryEvent is only ever run on the client side. Is that correct?

That is correct.

 

1 hour ago, jaminv said:

Some models require additional consideration. I'm asking if IHasCustomModel is an acceptable pattern when a block requires extra code to handle its models.

Unlikely. I would have to see a specific use-case scenario. I used something similar to handle items with a custom mesh definition as I was passing things through to a method that didn't have specific knowledge and needed to query a method on the Item passed to it. Though I could just as easily have use two separate objects.

[jaminv]

Well, the simple one is something like an `BlockOre` block where the ore type is stored in meta data. Instead of registering one model, I have historically iterated through the EnumOreType and called registerVariantRenderer() for each. Although thinking about it, I suppose I could just use a blockstate JSON file for this (as I do for the block itself).

 

A more complex example is a series of blocks that use IBakedModel rendering and uses metadata for rendering. I actually need a different IBakedModel for each metadata state, otherwise the block's particle texture will be the same. Therefore, I need to create a CustomStateMapper that iterates through the EnumMetadata and provide a separate ResourceLocation for each. I'm not sure there's a way around this one, (perhaps I'm missing something?)  and I'd really like to put this code with the objects themselves. This is specialized to the BakedModelProvider interface, so perhaps I could just add another method to the interface that returns an IStateMapper and defaults to a default implementation? Since I only currently need one VariantStateMapper to handle several similar block types, there wouldn't be any duplicate code.

[Draco18s]

1 hour ago, jaminv said:

Well, the simple one is something like an `BlockOre` block where the ore type is stored in meta data. Instead of registering one model, I have historically iterated through the EnumOreType and called registerVariantRenderer() for each. Although thinking about it, I suppose I could just use a blockstate JSON file for this (as I do for the block itself).

https://github.com/Draco18s/ReasonableRealism/blob/1.12.1/src/main/java/com/draco18s/hardlib/client/ClientEasyRegistry.java#L66-L71

But you should be flattening your blocks at this point. I don't care if you're still on 1.12, when you move to 1.13 you'll have to do it anyway

1 hour ago, jaminv said:

Therefore, I need to create a CustomStateMapper that iterates through the EnumMetadata and provide a separate ResourceLocation for each.

https://github.com/Draco18s/ReasonableRealism/blob/1.12.1/src/main/java/com/draco18s/hardlib/client/ClientEasyRegistry.java#L79-L97

[jaminv]

1 hour ago, Draco18s said:

https://github.com/Draco18s/ReasonableRealism/blob/1.12.1/src/main/java/com/draco18s/hardlib/client/ClientEasyRegistry.java#L66-L71

But you should be flattening your blocks at this point. I don't care if you're still on 1.12, when you move to 1.13 you'll have to do it anyway

That's basically what I'm doing now, although I didn't realize I could getValidStates(). That might help with some abstraction.

 

1 hour ago, Draco18s said:

https://github.com/Draco18s/ReasonableRealism/blob/1.12.1/src/main/java/com/draco18s/hardlib/client/ClientEasyRegistry.java#L79-L97

Ok, yes. That's almost exactly what I'm saying.

 

---

 

https://github.com/Draco18s/ReasonableRealism/blob/1.12.1/src/main/java/com/draco18s/farming/FarmingBase.java#L103-L137

 

It appears that you're not using the RegistryEvents to register your blocks/items? Is that not required?

 

I feel like a lot of the reason IHasModel ended up existing in the first place is to avoid disjointed code: where you've got instantiation in one place, registration in another, model registration in another, and then the block code somewhere else. So the tutorials I followed had a "instantiate the object, then have the instantiated object handle all its own registries" [anti]pattern.

It appears you just handle instantiation, registry, and (deferred) model registry in one place. That seems like a pretty good pattern. I think that's the missing piece I was looking for.

[Animefan8888]

12 minutes ago, jaminv said:

It appears that you're not using the RegistryEvents to register your blocks/items? Is that not required?

He is. Hes adding them to a list and then registering every value in that list. However I dont like that @Draco18s is creating his registry entries in preInit.

[jaminv]

I could just as easily instantiate them to an array and then registerAll() the array in the RegistryEvent.

 

What's the best place to instantiate them? PreInit? Does that happen before the RegistryEvent?

Edited August 26, 2019 by jaminv

[Animefan8888]

9 minutes ago, jaminv said:

Does that happen before the RegistryEvent?

Depends on the version you are using.

 

9 minutes ago, jaminv said:

What's the best place to instantiate them?

In their respective registry event.

[jaminv]

So optimally: In the RegistryEvent, call something like BlockInit.init(), which handle the instantiation, registration, and cache the model information?

 

EDIT: I guess there's no real reason to break off into another class.

Edited August 26, 2019 by jaminv

[Draco18s]

1 hour ago, Animefan8888 said:

However I dont like that @Draco18s is creating his registry entries in preInit.

Its still a Forge-controlled event. My reasons for not creating directly inside the Registry event is that I only need one registry event handler that supports four mods and keeps my main class looking clean and organized. All the crazy wackadoo "this block needs to do something special" code is tucked away either in that block, a block-adjacent class (eg an ItemBlock), or the EasyRegistry's methods.

[jaminv]

On 8/25/2019 at 1:14 PM, Draco18s said:

2) block instanceof BakedModelProvider is bad. BakedModelProvider is almost certainly client side only (or references something that's client side only) and will crash the dedicated server

I see what you mean now. While this did run properly on the dedicated server (due to layers on abstraction), when I'm doing everything properly it just isn't necessary. So much easier to just put the client code in a client proxy than having to worry about burying client code and/or accidentally referencing client objects in server code.

 

---

 

Man I wish the tutorials I found had told me that I could test the dedicated server using the run configurations that are automatically setup in Eclipse (and that I should do so regularly). I had to go specifically looking for that information. I ended up having a fluid that I missed an IHadModel on and the registerModels() function referenced IStateMapped directly in the Block class. I also had some client message handlers that referenced Minecraft.getMinecraft().world and needed to be registered in a client proxy as a result. I also discovered that JsonUtils.isString() is (for some unknown reason) client only. I've got everything cleaned up now.

 

I wonder if "test you code often on the dedicated server, here's how" should be in the common issues and recommendations thread?

 

---

 

Thanks for all the help. Turns out that it's just another way of thinking about it, but it's still possible to consolidate code without using IHasModel interfaces.

Edited August 27, 2019 by jaminv