Skip to content
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

[1.20.1 Fabric] Setting itemStack to null in EntityShapeContext breaks Bumblezone's brushable pollen block #498

Open
TelepathicGrunt opened this issue Nov 13, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@TelepathicGrunt
Copy link
Contributor

TelepathicGrunt commented Nov 13, 2023

Using lithium-fabric-mc1.20.1-0.11.2.jar and latest Bumblezone, attempting to brush a Suspicious Pile of Pollen block will do nothing.

The problematic mixin:

The issue comes down to me needing to use the EntityShapeContext's itemstack field directly so I can check if the item is in my brushing allowed tag. The isHolding method is no sufficient to my needs. However, Lithium sets this field to null which causes my block to never be brushed because it thinks player held item is null which is wrong.
https://github.com/TelepathicGrunt/Bumblezone/blob/cf5e4c67ba5de7713458651f05d888cc884b4752/common/src/main/java/com/telepathicgrunt/the_bumblezone/blocks/PileOfPollenSuspicious.java#L126

I originally got told about this issue because in past Bumblezone version, this conflict crashed my mod due to Canary copying the same mixin from lithium. I put out a null check fix but realized now that it means while the crash is stopped, the block is no longer brushable. I checked Fabric and Lithium also causes the issue with the mixin
AbdElAziz333/Canary#182
TelepathicGrunt/Bumblezone#299

I have suspicion on if this null field setting is even giving any tangible performance benefit... In general, itemstacks should never be null. As of now, I need to hack around Lithium to get the actual held item from the context again. What I may have to do is call isHolding directly once with a dummy input to force Lithium to init itemstack. Then get the field.

I wanted to grab the item directly from the context in case other mods make their own context with the itemstack set directly like a fake player or some sort of autobrushing machine which is why I can't assume that I can call ((LivingEntity)this.entity).getMainHandStack() myself.

@TelepathicGrunt TelepathicGrunt added the bug Something isn't working label Nov 13, 2023
@TelepathicGrunt
Copy link
Contributor Author

Also, if a mod tries to say automate brushing line-of-sight machine and does so by crafting their own EntityShapeContext to pass into brushable block's various methods, this mixin is assuming the itemstack is always ((LivingEntity)this.entity).getMainHandStack() which it could not be in this case and ends up overwriting the original itemstack that the machine was trying to pass into EntityShapeContext. basically this mixin makes lots of assumptions I am not comfortable with.

@2No2Name
Copy link
Member

I guess I can remove this optimization from lithium, I don't think it makes a large difference anyways

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants