-
Notifications
You must be signed in to change notification settings - Fork 109
Working 1.21 port (for the most part) #985
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
base: devel/port-1.21
Are you sure you want to change the base?
Conversation
# Conflicts: # Fabric/build.gradle # Forge/build.gradle # Forge/src/generated/resources/.cache/d4b3e6634c30118e1127c02b727ea285752e5aac # Neoforge/src/main/java/at/petrak/hexcasting/forge/loot/ForgeHexCypherLootMod.java
* Attempted fix of Networking (Incomplete) * Some minor fixes here and there
* Other fixes here and there (fixed up some whole minor folders so that's pretty cool) * Fixed NBTUtils!
* New ContainerInput for Recipes * Various edits to different Items * etc, this is setting up for a merge anyway
…visualizing on impeti scrying sight mishap) Fixed slate rendering both as a block entity and an item stack.
…n Fabric; might overhaul the registry with it. * Registered ItemApiLookups (ParticleSprays function on Fabric!!!) * Readded BrainsweepeeIngredient.getSomeKindOfReasonableIDForEmi(), EMI recipes work for the most part * Fixed Fabric Creative Tabs (mostly)
…r other things. * Fixed Fabric interop naming.
…(Not when erroring) * Adjusted Fabric creative mode tab entries (untested) * Fixed scrolls being unreadable * Fixed rendering artifacts when both toasts and casting stack are present on screen
* Fixed Media Cube implementation killing server * Adjusted neoforge.mods.toml
* Fixed ClientCastingStacks not ticking on Neo
…weep recipes, minor fixes on Fabric
…ve existed in the first place
|
Another note for future reviewers: if you use Bitwarden, apparently disabling its browser extension makes the review page work significantly faster??????? |
object-Object
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly only reviewed Common, will come back and look through the platform code later. Left many questions/comments/suggestions.
I think my main concern is with how EntityIota was ported. I'm not sure how we want it to work, but I'm not satisfied with how it currently is. We can discuss it more on Discord.
| compileKotlin { | ||
| compilerOptions { | ||
| jvmTarget = JvmTarget.JVM_21 | ||
| } | ||
| } | ||
| compileTestKotlin { | ||
| compilerOptions { | ||
| jvmTarget = JvmTarget.JVM_21 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already in subprojects, why do we need it here?
| pkSubproj { | ||
| platform = "common" | ||
| pkPublish = false | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary. We configure platform in the base build.gradle, and pkPublish defaults to false already.
| loom { | ||
| accessWidenerPath = file("src/main/resources/hexplat.accesswidener") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a duplicate.
| @@ -9,6 +10,7 @@ architectury { | |||
|
|
|||
| // FIXME: update dependencies/versions before releasing 0.12 | |||
| pkSubproj { | |||
| platform = "fabric" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already set in the root build.gradle
| modLocalRuntime "dev.architectury:architectury-fabric:$architecturyVersion" | ||
| modLocalRuntime "com.samsthenerd.inline:inline-fabric:$minecraftVersion-$inlineVersion" | ||
| modLocalRuntime "at.petra-k:paucal:$paucalVersion+$minecraftVersion-fabric" | ||
| localRuntime "hexcasting:jankson:1.2.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely we can get jankson from an actual maven? Why do we need to vendor it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the most reliable way via trial and error for me to prevent Fabric:runClient from failing due to a strange NoSuchMethodError related to Jankson; it's in the name 😭 (it randomly shows up or disappears, it's really weird)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but why do we need to include the jar locally? Jankson is on Maven Central. Also, can you put that context in a comment here?
| GameProfile HEXCASTING = new GameProfile(UUID.fromString("8BE7E9DA-1667-11EE-BE56-0242AC120002"), "[HexCasting]"); | ||
|
|
||
| boolean isBreakingAllowed(ServerLevel world, BlockPos pos, BlockState state, @Nullable Player player); | ||
|
|
||
| boolean isPlacingAllowed(ServerLevel world, BlockPos pos, ItemStack blockStack, @Nullable Player player); | ||
|
|
||
| <B> IXplatRegister<B> createRegistar(ResourceKey<Registry<B>> registryKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a doc comment
| import java.util.function.Supplier; | ||
|
|
||
| /** | ||
| * Handles registration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NeoForge registration uses deferred registry, fabric registration registers things directly, it's platform-independent interface useable in Common module
| * Handles registration | ||
| * @param <B> | ||
| */ | ||
| public interface IXplatRegister<B> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the type parameter called B?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was like that when i got here 😭, i can change it to T instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's B because it is BASE class for registry entry.
MyItem extends Item, where MyItem is T generic parameter in register method of this class and B is base class of registry, type parameter of the class
| LootTableEvents.MODIFY.register { key, tableBuilder, source, lookup -> | ||
| if (Blocks.AMETHYST_CLUSTER.lootTable.equals(key)) { | ||
| tableBuilder.modifyPools { pool -> | ||
| pool.apply(SetItemCountFunction.setCount(UniformGenerator.between(1.0f, 2.0f))) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Fabric version of changing the amethyst cluster's default drops for amethyst shards to half of its' original amount.
The 1.20 way of doing it no longer exists in 1.21, so I have to use FAPI to do it.
| HexTags.Actions.PER_WORLD_PATTERN | ||
| ) | ||
| ) keyList.add(key) | ||
| keyList.sortWith(Comparator.comparing<ResourceKey<ActionRegistryEntry?>?, ResourceLocation?>(Function { obj: ResourceKey<ActionRegistryEntry?>? -> obj!!.location() })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need !! here?

No description provided.