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

No null check for Minecraft.getInstance().player, causing crash #60

Closed
enjarai opened this issue May 14, 2022 · 6 comments
Closed

No null check for Minecraft.getInstance().player, causing crash #60

enjarai opened this issue May 14, 2022 · 6 comments

Comments

@enjarai
Copy link

enjarai commented May 14, 2022

I've noticed that in PlayerRendererMixin::setModelProperties you use Minecraft.getInstance().player without a null check.
image

Normally this wouldn't be a problem, since no players will get rendered unless the user has a world loaded. However, this causes issues when a mod tries to render a PlayerEntity while no world is loaded, which one of my mods does. See crash report here.

This could likely be fixed very easily by adding a null check that either: cancels the method, or preferably, skips the distance check that causes the issue.

@tr7zw
Copy link
Owner

tr7zw commented May 16, 2022

Will add a null-check in the next release.

@enjarai
Copy link
Author

enjarai commented Sep 22, 2022

I've recieved another crash report from a player having issues with a missing null check.

crash-2022-09-22_10.49.46-client.txt

After scanning through your code, I think the problem is on this line, but I'm not completely sure. You're probably more familiar with your own code.

if(livingEntity.distanceToSqr(Minecraft.getInstance().player) > SkinLayersModBase.config.renderDistanceLOD*SkinLayersModBase.config.renderDistanceLOD) {

Could you add another null check here? I can also try to find all problematic spots myself and open a PR if needed, but I might miss some.

@tr7zw
Copy link
Owner

tr7zw commented Sep 22, 2022

Looks like nl.enjarai.showmeyourskin.gui.widget.ArmorConfigWindow renders players while not inside a world, but sets the world and not the player? I had to add checks in the past for mods that render players in the main menu, but somehow that mod seems to break something about it?

@enjarai
Copy link
Author

enjarai commented Sep 22, 2022

I'm not sure if other mods modify client.player, but I don't, since that feels like something that would have unintented consequences. I don't know what else could be different about my mod compared to other mods that render players outside a normal world.

@tr7zw
Copy link
Owner

tr7zw commented Sep 22, 2022

Ok now I'm home and can actually look at the stack trace with mappings. The issue is that it's rendering a player head(the item) while not in a world.

I don't know what else could be different about my mod compared to other mods that render players outside a normal world.

This is in general an issue since there is no vanilla equivalent. So there is no right way to do it, and any way will cause other problems. You must have some dummy world to render that fake entity, so it could be argued that during the rendering, the Minecraft.level should be set to that. But then you have a level and no player, which is an invalid state, so adding a fake player there too(or having the rendered player be the fake player). This would fix issues with this mod, but might cause other mods to misbehave.

That's the reason I decided to not bother, and just don't show player previews while not ingame(3d skin layers/wave capes). Guess I'll add another null check for the next version, but it's more a bandaid than anything.

@enjarai
Copy link
Author

enjarai commented Sep 22, 2022

I've had surprisingly few reports about incompatibilities with mods that don't nullcheck client.player, so its not a big problem imo. But like you say, there's no one size fits all solution to this.

I'll probably add a "compatibility mode" or something to my mod in the future, making it disable the previews when no world is loaded. By default they'll still be enabled though, since they're quite important to my UI design.

Its fully understandable if you don't want to bother supporting this, but I'd appreciate it if you do. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants