Jump to content

[SOLVED] IHasModel alternative for more complex situations


jaminv

Recommended Posts

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 by jaminv
Link to comment
Share on other sites

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 by Draco18s

Apparently I'm a complete and utter jerk and come to this forum just like to make fun of people, be confrontational, and make your personal life miserable.  If you think this is the case, JUST REPORT ME.  Otherwise you're just going to get reported when you reply to my posts and point it out, because odds are, I was trying to be nice.

 

Exception: If you do not understand Java, I WILL NOT HELP YOU and your thread will get locked.

 

DO NOT PM ME WITH PROBLEMS. No help will be given.

Link to comment
Share on other sites

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 by jaminv
Link to comment
Share on other sites

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.

Apparently I'm a complete and utter jerk and come to this forum just like to make fun of people, be confrontational, and make your personal life miserable.  If you think this is the case, JUST REPORT ME.  Otherwise you're just going to get reported when you reply to my posts and point it out, because odds are, I was trying to be nice.

 

Exception: If you do not understand Java, I WILL NOT HELP YOU and your thread will get locked.

 

DO NOT PM ME WITH PROBLEMS. No help will be given.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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

Apparently I'm a complete and utter jerk and come to this forum just like to make fun of people, be confrontational, and make your personal life miserable.  If you think this is the case, JUST REPORT ME.  Otherwise you're just going to get reported when you reply to my posts and point it out, because odds are, I was trying to be nice.

 

Exception: If you do not understand Java, I WILL NOT HELP YOU and your thread will get locked.

 

DO NOT PM ME WITH PROBLEMS. No help will be given.

Link to comment
Share on other sites

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:

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.

Link to comment
Share on other sites

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.

VANILLA MINECRAFT CLASSES ARE THE BEST RESOURCES WHEN MODDING

I will be posting 1.15.2 modding tutorials on this channel. If you want to be notified of it do the normal YouTube stuff like subscribing, ect.

Forge and vanilla BlockState generator.

Link to comment
Share on other sites

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 by jaminv
Link to comment
Share on other sites

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.

VANILLA MINECRAFT CLASSES ARE THE BEST RESOURCES WHEN MODDING

I will be posting 1.15.2 modding tutorials on this channel. If you want to be notified of it do the normal YouTube stuff like subscribing, ect.

Forge and vanilla BlockState generator.

Link to comment
Share on other sites

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 by jaminv
Link to comment
Share on other sites

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.

Apparently I'm a complete and utter jerk and come to this forum just like to make fun of people, be confrontational, and make your personal life miserable.  If you think this is the case, JUST REPORT ME.  Otherwise you're just going to get reported when you reply to my posts and point it out, because odds are, I was trying to be nice.

 

Exception: If you do not understand Java, I WILL NOT HELP YOU and your thread will get locked.

 

DO NOT PM ME WITH PROBLEMS. No help will be given.

Link to comment
Share on other sites

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 by jaminv
Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.
Note: Your post will require moderator approval before it will be visible.

Guest
Unfortunately, your content contains terms that we do not allow. Please edit your content to remove the highlighted words below.
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

Announcements



  • Recently Browsing

    • No registered users viewing this page.
  • Posts

    • They were already updated, and just to double check I even did a cleanup and fresh update from that same page. I'm quite sure drivers are not the problem here. 
    • i tried downloading the drivers but it says no AMD graphics hardware has been detected    
    • Update your AMD/ATI drivers - get the drivers from their website - do not update via system  
    • As the title says i keep on crashing on forge 1.20.1 even without any mods downloaded, i have the latest drivers (nvidia) and vanilla minecraft works perfectly fine for me logs: https://pastebin.com/5UR01yG9
    • Hello everyone, I'm making this post to seek help for my modded block, It's a special block called FrozenBlock supposed to take the place of an old block, then after a set amount of ticks, it's supposed to revert its Block State, Entity, data... to the old block like this :  The problem I have is that the system breaks when handling multi blocks (I tried some fix but none of them worked) :  The bug I have identified is that the function "setOldBlockFields" in the item's "setFrozenBlock" function gets called once for the 1st block of multiblock getting frozen (as it should), but gets called a second time BEFORE creating the first FrozenBlock with the data of the 1st block, hence giving the same data to the two FrozenBlock :   Old Block Fields set BlockState : Block{minecraft:black_bed}[facing=east,occupied=false,part=head] BlockEntity : net.minecraft.world.level.block.entity.BedBlockEntity@73681674 BlockEntityData : id:"minecraft:bed",x:3,y:-60,z:-6} Old Block Fields set BlockState : Block{minecraft:black_bed}[facing=east,occupied=false,part=foot] BlockEntity : net.minecraft.world.level.block.entity.BedBlockEntity@6d1aa3da BlockEntityData : {id:"minecraft:bed",x:2,y:-60,z:-6} Frozen Block Entity set BlockState : Block{minecraft:black_bed}[facing=east,occupied=false,part=foot] BlockPos{x=3, y=-60, z=-6} BlockEntity : net.minecraft.world.level.block.entity.BedBlockEntity@6d1aa3da BlockEntityData : {id:"minecraft:bed",x:2,y:-60,z:-6} Frozen Block Entity set BlockState : Block{minecraft:black_bed}[facing=east,occupied=false,part=foot] BlockPos{x=2, y=-60, z=-6} BlockEntity : net.minecraft.world.level.block.entity.BedBlockEntity@6d1aa3da BlockEntityData : {id:"minecraft:bed",x:2,y:-60,z:-6} here is the code inside my custom "freeze" item :    @Override     public @NotNull InteractionResult useOn(@NotNull UseOnContext pContext) {         if (!pContext.getLevel().isClientSide() && pContext.getHand() == InteractionHand.MAIN_HAND) {             BlockPos blockPos = pContext.getClickedPos();             BlockPos secondBlockPos = getMultiblockPos(blockPos, pContext.getLevel().getBlockState(blockPos));             if (secondBlockPos != null) {                 createFrozenBlock(pContext, secondBlockPos);             }             createFrozenBlock(pContext, blockPos);             return InteractionResult.SUCCESS;         }         return super.useOn(pContext);     }     public static void createFrozenBlock(UseOnContext pContext, BlockPos blockPos) {         BlockState oldState = pContext.getLevel().getBlockState(blockPos);         BlockEntity oldBlockEntity = oldState.hasBlockEntity() ? pContext.getLevel().getBlockEntity(blockPos) : null;         CompoundTag oldBlockEntityData = oldState.hasBlockEntity() ? oldBlockEntity.serializeNBT() : null;         if (oldBlockEntity != null) {             pContext.getLevel().removeBlockEntity(blockPos);         }         BlockState FrozenBlock = setFrozenBlock(oldState, oldBlockEntity, oldBlockEntityData);         pContext.getLevel().setBlockAndUpdate(blockPos, FrozenBlock);     }     public static BlockState setFrozenBlock(BlockState blockState, @Nullable BlockEntity blockEntity, @Nullable CompoundTag blockEntityData) {         BlockState FrozenBlock = BlockRegister.FROZEN_BLOCK.get().defaultBlockState();         ((FrozenBlock) FrozenBlock.getBlock()).setOldBlockFields(blockState, blockEntity, blockEntityData);         return FrozenBlock;     }  
  • Topics

×
×
  • Create New...

Important Information

By using this site, you agree to our Terms of Use.