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.14.4/optifine magma block causes NPE #69

Closed
wasche opened this issue Nov 18, 2019 · 10 comments
Closed

1.14.4/optifine magma block causes NPE #69

wasche opened this issue Nov 18, 2019 · 10 comments
Labels
Bug Something that shouldn't be that way Fixed The problem has been fixed

Comments

@wasche
Copy link

wasche commented Nov 18, 2019

Not sure if it is optifine related, but I'm guessing so. Was doing some mining and hit a cave, turning on the light level overlay immediately causes an NPE.

crash report: crash-2019-11-18_08.30.23-client.txt

@feldim2425 feldim2425 added Bug Something that shouldn't be that way high priority This problem breaks the game and should be fixed as quick as possible labels Nov 18, 2019
@feldim2425
Copy link
Owner

I can reproduce it without optifine. The problem seems to be something about getting the spawn information from magma blocks. Just turning on the overlay and placing a magmablock in rage of the overlay is enough to crash immediately.

@feldim2425
Copy link
Owner

After a quick look into the source code for the magmablock it either seems that it's canEntitySpawn method was programmed without keeping in mind that the EntityType can be null or the method is documented wrong since they marked that parameter as Nullable.

The fix sadly reduces performance a bit since I have to check every singe monster entitytype that might spawn. So I added a config option to make a simple check just for the Zombie entity.

@feldim2425 feldim2425 added Fixed The problem has been fixed and removed high priority This problem breaks the game and should be fixed as quick as possible labels Nov 18, 2019
@noobanidus
Copy link

That's interesting, I was just coming to report this and was checking through closed issues to see if it had already been discussed. It got brought up on our discord and there was some conversation about it.

I did have a look at the decompiled code from IntelliJ and interestingly enough it doesn't have a @Nullable notation on the EntityType parameter in either the BlockState implementation or the actual Block implementation. I'm not sure if my code has had it stripped out, however.

Of note, though, it seems that BlockMagma is one of the few that actually implement canEntitySpawn. I'm not sure whether that's helpful or not, though, as that can't really be used as a heuristic given modded blocks exist...

I personally would definitely recommend making the Zombie EntityType default, however, and adding an option for the "test every monster entitytype".

@feldim2425
Copy link
Owner

Here is a link to the source: https://github.com/MinecraftForge/MinecraftForge/blob/6422310453fe4029a409adf19d3bac6bfb3ea589/src/main/java/net/minecraftforge/common/extensions/IForgeBlock.java#L269

IForgeBlock is implemented by Block because Forge uses a .patch file to add it. And I used that method to determine if mobs can spawn.

@wasche
Copy link
Author

wasche commented Nov 19, 2019

Thanks for the super fast fix! Any idea what release this'll be in? Just curious, I've no problems building it for myself, but its nice to be lazy and just click the button in the launcher :)

I'd also second making zombie default and the option to check all.

@feldim2425
Copy link
Owner

feldim2425 commented Nov 19, 2019

Already out in 1.16.1 alpha release on curseforge. Along with some other fixes and features.
https://www.curseforge.com/minecraft/mc-mods/more-overlays/files/2829171

EDIT: There it is still alpha since there are some more things planed for 1.14.4.

  • The config gui will get it's own list system since the vanilla AbstractList doesn't fit very well from the way it handles gui events, rendering and init-calls
  • I want to add some way to open the config ui ingame either by adding a button to the pause screen or via a client side command
  • The current system to detect mob spawns should get more options to set it into light only mode and maybe find a better way to detect mobspawns without checking all mobs.
  • I want to add IMC messages for the API.

@noobanidus
Copy link

noobanidus commented Nov 20, 2019

That's definitely a bug on Forge's part, then, because neither the function they call in the state nor the function that it calls in the block will accept a null entity type... I might go file a bug.

EDIT: Especially demonstrable by the fact that the Vanilla magma block is accessing the EntityType directly without testing for null first.

EDIT 2: I've filed a Forge issue for this: MinecraftForge/MinecraftForge#6326

@feldim2425
Copy link
Owner

Also I should add that only checking for the EntityType "Zombie" has the problem, that blocks like the magma block won't show any overlay, since they only allow monsters that resist fire, thats why I currently set the default to check all mobs, because usually users will complain that the overlay doesn't work when they just had to change to the less performant setting.
I will try to add more options like a true ligt only mode and maybe I or someone else finds a better way to optimize the check (maybe caching the blocks spawn mode).

@noobanidus
Copy link

noobanidus commented Nov 20, 2019

Perhaps Zombie Pigman could be an alternative to zombie? I believe they should satisfy most conditions.

Additionally, a block(state?) -> boolean conditional map that could be cached might also help to simplify things, or at least speed up the process.

EDIT: Judging by the adjustment of tags to "1.14" and "cleanup", I'm going to presume that the "@Nullable" annotation remaining was an oversight from 1.12, where I presume it was accepting an Entity(Living?) instead of an EntityType.

@feldim2425
Copy link
Owner

In 1.12 there was no extra parameter at least not in the method I used. It simply required the BlockState and the SpawnPlacementType. I don't know if spawns based on State, Placement and Entity were a thing in 1.12 but I assume "no".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something that shouldn't be that way Fixed The problem has been fixed
Projects
None yet
Development

No branches or pull requests

3 participants