bconlon1
commented
2 years ago ItemStackMixin
Yeah I plan to remove this whenever I can do our item refactors when https://github.com/MinecraftForge/MinecraftForge/pull/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).
Jonathing
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
glorydays 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.
INVOKEinstruction and inject code after that instruction, do not use@At(value = "INVOKE", shift = At.Shift.AFTER). Instead, use@At(value = "INVOKE_ASSIGN".shiftaltogether. Always attempt to target a specific instruction unless you have ground to shift the injection point otherwise.methodparameter of@Injector 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 inRight Click -> Copy / Paste Special -> Mixin Target Reference. Unlike the method description, the method owner (the class name) is not required.:heavy_check_mark: 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.@At()annotation, be sure to mark any non-Minecraft methods asremap = 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.:heavy_check_mark: AbstractFurnaceBlockEntityMixin
Again, this mixin can be easily cleaned up by just overriding the
burn()method from the superclass.:heavy_check_mark: 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 inAetherDimensionswith its public constructor. Simply use an anonymous class to override thetimeOfDay()andmoonPhase()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.:heavy_check_mark: LivingEntityMixin
In the
INVOKEcall toaiStep()(which should beINVOKE_ASSIGNlike I mentioned in general points earlier), you are executing code afteraiStep()has run, where it is the only time it is run in thetick()method. Why not just Inject into that method at@At("RETURN")?:heavy_check_mark: 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 :trollface:). 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