search

YUNG-GANG / YUNGs-Better-Strongholds

Minecraft mod for enhancing vanilla strongholds
GNU Lesser General Public License v3.0
23 stars 11 forks source link

[1.17.1] Overwriting vanilla stronghold /locate command #34

Closed kwpugh closed 3 years ago

kwpugh commented 3 years ago

Hi,

Can I make a suggestion regarding the following mixin?

https://github.com/yungnickyoung/YUNGs-Better-Strongholds/blob/a8877c71a268618bc9be593a06e5a04ba0221d2f/src/main/java/com/yungnickyoung/minecraft/betterstrongholds/mixin/LocateStrongholdCommandMixin.java

This mixin is overriding the built-in vanilla locate with a message, which is fine for end user, but creates a null pointer exception for vanilla methods that rely on it.

I do not completely understand why, but it seems that your mixin is changing the value of any use of "StructureFeature.STRONGHOLD" to return null because it is not allowed to assign it.

Can I suggest rather than replacing it with a text message that you assign your betterstrongholds:stronghold to the vanilla one as well, that way anything existing code that is using StructureFeature.STRONGHOLD will not break and will use your structure instead? It appears that your intent is to complete replace the vanilla stronghold, so it would make sense to replace the command as well.

In the mean, I will put a null check in my stronghold locater, but the result is still that it will not function unless I hard-code something to look for your replacement of it.

If I am misunderstanding what is happening here (which might be true since I'm still learning programming) please let me know, or if there is another way around this, let me know that as well.

Thank you for considering.

Kevin

yungnickyoung commented 3 years ago

Sorry for the late response. I don't think I ever set the field StructureFeature.STRONGHOLD. Rather, I think what's happening is that the locate method cannot find any vanilla strongholds (since none exist in the world when using Better Strongholds), and so the method call returns null for the BlockPos. This is intended behavior of the locateStructure method in ChunkGenerator, so you should be adding a null check before using the return value anyway. I don't think there's any way for me to get around this without a complicated mixin changing the behavior of this method, which isn't necessary IMO.

kwpugh commented 3 years ago

Yeah, I had already added a null check when it was reported. Thanks anyways.