Mixins

are bad!

Anyways

Mixins are actually great (I've been trolling you the whole time). They provide a stable API to modifying bytecode that we otherwise wouldn't be able to. Of course, using Mixins still require some knowledge of how bytecode works to properly use them, but nonetheless Mixins are so easy and useful that they remind me of the glory days of jarmodding.

However using Mixins (or really any form of coremodding) can be really dangerous, because they pose the threat of causing behaviors that would otherwise not be accounted for. The reason for this is because Forge's entire existence relies on mod compatibility through the use of the Forge API, unlike stripped down modloaders like Fabric which rely heavily on the usage of coremodding. It's because of this that mixins should be written as cleanly, efficiently, and effectively as possible, while also keeping their numbers as low as possible so as to not add behaviors that could otherwise break the fuck out of the game.

Disclaimer

I'm an asshole. I apologize if I come off as rude in this issue, but I want to be as thorough as possible with my critiques of these mixins. Remember that faulty coremodding can effect everybody, not just the mod at hand.

Let's Begin

General Points

There are a couple of general points that apply to either more than one mixin, or are general enough that they should always be followed as a rule of thumb.

• If you need to target an INVOKE instruction and inject code after that instruction, do not use @At(value = "INVOKE", shift = At.Shift.AFTER). Instead, use @At(value = "INVOKE_ASSIGN".
  • Actually to add on to that, I do not recommend using shift altogether. Always attempt to target a specific instruction unless you have ground to shift the injection point otherwise.
• Although this doesn't cause any issues at the current moment, I would not recommend using only the method name in the method parameter of @Inject or similar annotations. Always include the full method description as well. IntelliJ has a built-in Bytecode Viewer so you can easily copy the description from the viewer, but you can also copy the entire Mixin target reference in Right Click -> Copy / Paste Special -> Mixin Target Reference. Unlike the method description, the method owner (the class name) is not required.

✔️ ItemStackMixin

This seems like kind of a redundant mixin. You can easily modify an itemstack's default attributes by overriding the getDefaultInstance() method in the itemclass, and providing your own default itemstack.

• Additionally, I'm getting a compiler warning saying that it can't find the srg name for a forge method. In the @At() annotation, be sure to mark any non-Minecraft methods as remap = false.

AbstractArrowMixin

This mixin is brute forcing additional functionalities for the Pheonix Arrow that could otherwise just be added with a custom implementation of AbstractArrow using a custom projectile and then overriding the tick() method. In fact if you created your own projectile, this entire capability might be completely redundant. See what you can work with, though. Sometimes vanilla is very finnicky with this shit.

✔️ AbstractFurnaceBlockEntityMixin

Again, this mixin can be easily cleaned up by just overriding the burn() method from the superclass.

✔️ DimensionTypeMixin

While this might not have been the case in 1.18, this mixin is now completely redundant because you are creating the Dimension type in AetherDimensions with its public constructor. Simply use an anonymous class to override the timeOfDay() and moonPhase() methods.

just kidding that's a datagen cope

FeatureMixin

It's not very clear what you are trying to accomplish with this mixin. The method isGrassOrDirt() is not used anywhere in Forge or vanilla (simply CTRL + Click the method name to see for yourself) (this might not have been the case before 1.18). Also, it is checking if the block state is part of the dirt tag. Since the aether dirt tag is being added to the dirt tag, this should not be an issue.

✔️ LivingEntityMixin

In the INVOKE call to aiStep() (which should be INVOKE_ASSIGN like I mentioned in general points earlier), you are executing code after aiStep() has run, where it is the only time it is run in the tick() method. Why not just Inject into that method at @At("RETURN")?

✔️ ConnectScreenMixin, CreateWorldScreenMixin, and a shit ton of others relating to this main menu world gui

This mixin is actually fine, and I would encourage that you keep using it. This niche case of loading the world in the title screen is so specific that you will want full control over how this happens so it doesn't fuck with the game accidentally.

Although, it might be prudent to attempt to search for a way to tie the main menu world with the main menu GUI, so as to not rely on mixins based off of timing.

That's all folks!

Yikes, that was a bit. As you might have noticed, a good amount of these were nitpicks, while others were potential concerns. In any case, the point wasn't to shit on you guys for using potentially faulty mixins (even though I did that ). The point was to convey just how dangerous these kinds of mixins are because they can potentially disrupt the flow of other mods that achieve similar goals.

ok time to eat lunch fuck off

ItemStackMixin

Yeah I plan to remove this whenever I can do our item refactors when MinecraftForge/MinecraftForge#8891 is merged. The implementation I was using it for ended up being too messy.

AbstractArrowMixin

The point of the capability and mixin is because Phoenix Arrows are not a type of arrow, they're more of a modifier for any existing arrow shot by the Phoenix Bow, so it can't be its own class. So if I weren't using a mixin I would need some other way to attach particles to shot arrows without having a tick event for non-living entities.

AbstractFurnaceBlockEntityMixin

Yeah I have to look into removing this as well, I need to check though if there are still any weird issues with overriding burn() as caused by forge patches.

FeatureMixin

I mentioned before that isGrassOrDirt() is used by AlterGroundDecorator. This mixin stops large spruce trees from creating podzol in the Aether basically. However, I may look into PRing a tag to Forge to allow a better solution than this mixin.

LivingEntityMixin

Yeah good point I suppose that'd make more sense. (You didn't mention the other mixin in this class but I'll just say the other one in that class I plan to remove probably as well since its in a similar situation to ItemStackMixin).

Based

MinecraftForge/MinecraftForge#8360 may allow for phasing out the advancement sound mixin.

Mixins:

• AbstractClientPlayerMixin
• AdvancementToastMixin
• BossHealthOverlayMixin
• ConnectScreenMixin
• CreateWorldScreenMixin
• ElytraLayerMixin
• LocalPlayerMixin
• PanoramaRendererMixin
• PauseScreenMixin
• TippableArrowRendererMixin
• TitleScreenMixin
• WorldListEntryMixin
• WorldOpenFlowsMixin
• AbstractArrowMixin
• AbstractFurnaceBlockEntityMixin
• BeeGrowCropGoalMixin
• CropBlockMixin
• DimensionTypeMixin
• DirectoryLockMixin
• FeatureMixin
• FoxEatBerriesGoalMixin
• ItemStackMixin
• ItemMixin
• LivingEntityMixin
• PlayerMixin
• I should annotate mixins that will have neoforge PRs to replace them in 1.20.1+ as marked for deletion, like FeatureMixin.

I am considering this resolved as of commits c8b9a06, 4d9b952, c6f1e78, f34c104, and 03766f8 on branch feat/bconlon/cleanup. The overall state of mixins in the mod is much better than it once was before, though I'll let GitHub close this issue itself once the cleanup branch is merged.. As I mentioned on Discord, I take some slight issue with the semantics of ItemMixin, and am working a PR on my own to address how it's done and a potential refactor and rework.

The unofficial sequel: Zepalesque/The-Aether-Redux#60