[1.21.5] Add item model/item renderer related events #10528
Conversation
…sterColorHandlersEvent.Item and RegisterItemModelPropertiesEvent
|
|
|
This PR needs game tests to validate that these events are working as intended with examples. You can find our game tests in the |
Done :) Now if you run forge client test you'll notice 6 new items, and they are proof that each of the 6 new events works properly |
There was a problem hiding this comment.
I probably won't get around to properly reviewing this for a little while. But I'm kind of on the fence with this PR. It's been on the back-burner since I've been working on the toolchain, but I've wanted to make some sort of proper, simple API for working with the new model system that was introduced in 1.21.3, and subsequently expanded upon with item models in 1.21.4. So, I'm not the biggest fan of the "let's add an event here and hope people use it!" mentality, but for this it might have to do. We'll see some time later.
Also, please allow me to make clear now that all of my comments are not an attempt to dismiss your work. I am doing my best to make sure that all PRs that are submitted to Forge, and thus reviewed, are done so consistently, as simple as possible, and with as few issues as possible. I understand that sometimes my review comments may come across as harsh, but I'd rather make sure you understand my issues with something rather than sugarcoat it and risk something being lost in translation.
A few key things to note:
- Your test mod does not actually include game tests. Game tests include code that are executed with the
/testcommand, similar to unit tests. I know Minecraft kind of destroyed how game tests are registered in 1.21.5, but if Lex got them working, please add some of your own. What I expect is for you to be able to useMinecraft.getInstance().getModelManager().getItemModel(key)(or similar if this was removed in 1.21.5) to verify that the model you got is using the correct type and has the correct details. - The formatting you are using for this PR is outdated and inconsistent (some classes use either or).
- Please stick to bracket-on-same-line style.
- I prefer that short Javadoc comments be single-lined (
/** like this */).
| IEventBus bus = ctx.getModEventBus(); | ||
| DeferredRegister<Item> register = DeferredRegister.create(ForgeRegistries.ITEMS, modid()); | ||
| for (String s: new String[] {"your_head", "warden_plushy", "yellow_cap_covered_in_snow", "rod_of_game_mode", "swim_with_this", "fullinventorymeter"}) { | ||
| RegistryObject<Item> registryObject = register.register(s, () -> new Item(name(modid(), s, new Item.Properties()))); | ||
| testItem(provider -> registryObject.get().getDefaultInstance()); | ||
| } | ||
| register.register(bus); | ||
| bus.register(ItemModelsTest.class); |
There was a problem hiding this comment.
If you look in BaseMod, all fields of type DeferredRegister are automatically registered for you. You can create a DeferredRegister field in this class without needing to register it yourself. Please do the same for your registry objects, so that they can be directly referenced in your tests.
There was a problem hiding this comment.
This file should only be this:
clientSideOnly=trueThere was a problem hiding this comment.
You should use a data generator to make this file. See ModelRenderLayerTest for example generators.
There was a problem hiding this comment.
You should use a data generator to make this file. See ModelRenderLayerTest for example generators.
There was a problem hiding this comment.
You should use a data generator to make this file. See ModelRenderLayerTest for example generators.
There was a problem hiding this comment.
You should use a data generator to make this file. See ModelRenderLayerTest for example generators.
There was a problem hiding this comment.
You should use a data generator to make this file. See ModelRenderLayerTest for example generators.
There was a problem hiding this comment.
You should use a data generator to make this file. See ModelRenderLayerTest for example generators.
| public static class Item extends RegisterColorHandlersEvent | ||
| { | ||
|
|
||
| @ApiStatus.Internal | ||
| public Item() {} | ||
|
|
||
| /** | ||
| * Register an {@linkplain ItemTintSource item tint source} {@linkplain MapCodec map codec} with a specific id. | ||
| */ | ||
| public void register(ResourceLocation id, MapCodec<? extends ItemTintSource> codec) { | ||
| ItemTintSources.ID_MAPPER.put(id, codec); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Please use consistent formatting with the rest of the class file.
There was a problem hiding this comment.
You should replace the parameter srg names (shit like p_12345_) with an actual name, to make the code easier to read.
|
Also I realized that several times while reviewing this, I mistakenly thought this was for 1.21.4. Can you prefix [1.21.5] to the beginning of your PR title? Thanks 😎👍 |
CheIlia
changed the title
CheIlia
changed the title
Hello! I've changed code formatting and deleted item model test assets as you asked. Datagen doesn't work for now, and there are still no game tests, but I'll work on that soon. Also, I totally understand that it might seem like the item model event system could've been done easier, without creating .register() method in vanilla classes. I tried calling the events directly inside ItemModels, SpecialModelRenderers, ItemTintSources, SelectItemModelProperties, ConditionalItemModelProperties and RangeSelectItemModelProperties, but all these classes are called only during bootstrap, and forge "cowardly refuses to send events to a broken mod state" because it's too early to send events. Thus, these events have to be posted in a different class. I made it so that they are called in ForgeHooksClient.initClientHooks(), but turns out that method is never fired when running datagen, so I'll probably have to call them in ClientModLoader and DatagenModLoader instead |
|
Alright, I've fixed datagen and added an actual game test. Now everything should be done, I guess. Well, if you will, I can delete the new register() methods in ItemModels, SpecialModelRenderers, etc. and just use an access transformer to make the id mappers public |
IClientItemExtensions.getCustomRenderer() has been gone since 1.21.4 (if not earlier), and there is no way to make custom item renderers without using access transformers or mixins at the moment. This pull request makes it possible again, as well as adds a couple new events that allow users to register important item model related stuff