SlimeKnights / TinkersConstruct

Tinker a little, build a little, tinker a little more...
MIT License
1.24k stars 785 forks source link

Port to 1.20.1 #5258

Open ToCraft opened 2 months ago

ToCraft commented 2 months ago

I've split it into multiple commits so you can check each commit sperately. There are still some compiler errors, but I think you can already take a look at the current state :smiley:


I think at least the following is worth of discussing:


Some of the things I still haven't taken a look at:

ToCraft commented 2 months ago

I don't know why, but GitHub just closed the PR after I've tried to rename the branch (I'm used to GitHub auto-updating every reference, seems like it didn't work this time)

KnightMiner commented 2 months ago

Sometimes I've used BuiltInRegistries and ForgeRegistries, but Forge wants modders to only use ForgeRegistries when possible afaik

Forge Registries are deprecated, they are removed in 1.21 Neo. I know Forge claims vanilla registries are deprecated, but thats only because the Forge team had no plans to remove them; functionally there has never been a reason to not use the vanilla ones.

rn CreativeTabs collect their items by adding the item to a list when it's registered (see addToTabItemList(item, list) )

Yeah, this is one of those things that makes it hard to review port PRs, such decisions are ones I need to decide how I want it done rather than the code porting over one by one.

rn The Tag Providers get a list of Elements and get their keys by iterating. Maybe there's a more direct way to get the ids (e.g. getBlocksResourceKeys(blocks) )

I don't know what you mean by this, but it sounds like the wrong approach. I've been watching Forge and never seen any hacks needed to continue tag datagen.

The DataProvider's run method can now return a CompleteableFuture, maybe some stuff should therefore be run asynchronously?

Look at how Mantle updated its data generators.

There are still some compiler errors, but I think you can already take a look at the current state 😃

Its really hard to review it properly with compile errors. It makes sense when I am porting on my own, but not when someone else is porting it as I never know if code is the way it is because it has to be or its just not updated.

ToCraft commented 2 months ago

Forge Registries are deprecated, they are removed in 1.21 Neo. I know Forge claims vanilla registries are deprecated, but thats only because the Forge team had no plans to remove them; functionally there has never been a reason to not use the vanilla ones.

I've changed it as of f0806b5380a1752287e52d3c35849e92d626284e, but in some places I still had to use ForgeRegistries because I could only find some registries there and some methods require ForgeRegistries instead of vanilla registries.

Yeah, this is one of those things that makes it hard to review port PRs, such decisions are ones I need to decide how I want it done rather than the code porting over one by one.

You have write access in the PR- feel free to adjust it according to your needs. Can you also take a look at the implementations of Item.fillItemCategory and RetexturedBlockItem? I'm not sure how you'd like to have it ported.

I don't know what you mean by this, but it sounds like the wrong approach. I've been watching Forge and never seen any hacks needed to continue tag datagen.

It's not that wrong, it's just in case of the BlockEntityTypeTagProvider, you've specified BlockEntityType[] but in 1.20.1 ResourceKey<BlockEntityType>[] is required. That's why I wrote a function that converts your BlockEntityType[] into ResourceKey<BlockEntityType>[] using the Registries.

Look at how Mantle updated its data generators.

Alright, I've adapted it as of e036752cb903314a77556e9880ee8126c8aa01af but I'm not sure whether the callbacks should be run asynchronious, too. (Take a look at AbstractMaterialRenderInfoProvider.run(cache) for details)

Its really hard to review it properly with compile errors. It makes sense when I am porting on my own, but not when someone else is porting it as I never know if code is the way it is because it has to be or its just not updated.

I thought I would create the PR now so that we can coordinate better. But since it is not 100% fixed yet, it is only a draft PR so far.

spppacecat commented 1 month ago

the finishing is armour trims ?

KnightMiner commented 1 month ago

the finishing is armour trims ?

While armor trims are a feature desired in 1.20, its out of scope in a porting PR so should be left out. We have systems in place that will let us support them on our models, though that requires me comparing our models to vanilla changes to decide the best approach.