Skip to content

Fixed NullPointerException on startup #730

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

Open
wants to merge 1 commit into
base: dev/1.20.4
Choose a base branch
from

Conversation

philipp-gatzka
Copy link

@philipp-gatzka philipp-gatzka commented Apr 25, 2025

PLEASE READ THE GUIDELINES BEFORE MAKING A CONTRIBUTION

  • Please check if the PR fulfills these requirements
  • The commit message are well described
  • Docs have been added / updated (for features or maybe bugs which were noted). If not, please update the needed documentation here. Feel free to remove this check if you don't need it
  • All changes have fully been tested
  • What kind of change does this PR introduce? (Bug fix, feature, ...)
    Bug Fix

  • What is the current behavior? (You can also link to an open issue here)

When trying run minecraft 1.20.4 with neoforge and AP installed

Loading throws ´java.lang.NullPointerException: Cannot invoke "net.neoforged.neoforge.registries.DeferredHolder.get()" because "de.srendi.advancedperipherals.common.setup.BlockEntityTypes.ME_BRIDGE" is null

is thrown
This is because APAddons::commonSetup is called after Registration::register

public static final DeferredHolder<BlockEntityType<?>, BlockEntityType<MeBridgeEntity>> ME_BRIDGE = APAddons.ae2Loaded ? Registration.BLOCK_ENTITIES.register("me_bridge", () -> new BlockEntityType<>(MeBridgeEntity::new, Sets.newHashSet(Blocks.ME_BRIDGE.get()), null)) : null;

the issue is, that APAddons.ae2Loaded has not been initialized so it defaults to false

Default Value: The default value of a boolean variable is false

  • What is the new behavior (if this is a feature change)?

Whenever something checks if a mod is loaded, the ModList will directly be checked

  • Does this PR introduce a breaking change? (What changes might users need to make in their scripts due to this PR?)

i dont think so :D

  • Other information:

Copy link
Member

@SirEndii SirEndii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preferred way to fix this would be to rename the commonSetup function to setup and to move it into the constructor at the first place
At this point, the mod list is already initialized

I am currently only at my phone. So I am unsure if the preview works.

https://github.com/IntelligenceModding/AdvancedPeripherals/blob/dev%2F1.21.1/src%2Fmain%2Fjava%2Fde%2Fsrendi%2Fadvancedperipherals%2FAdvancedPeripherals.java#L32

@zyxkad
Copy link
Collaborator

zyxkad commented Apr 25, 2025

I think we can put that to static class init block instead of commonSetup?

@philipp-gatzka
Copy link
Author

Why not use ModList.get().isLoaded(....) where APAddons.....Loaded is used?

@zyxkad
Copy link
Collaborator

zyxkad commented Apr 25, 2025

I wonder if it may has less performance than check a boolean, but I may over thinking it.

@philipp-gatzka
Copy link
Author

I wonder if it may has less performance than check a boolean, but I may over thinking it.

Well ModList.get() just returns its singleton instance; private static ModList INSTANCE
.isLoaded just runs a containsKey on a map

so i dont think that this will have any performance impact

@SirEndii
Copy link
Member

SirEndii commented Apr 25, 2025

I wonder if it may has less performance than check a boolean, but I may over thinking it.

Maybe a bit. But saving that as a variable in a field like we do it currently is nothing harder to do than calling ModList... everytime. So I just prefer caching the mod loaded state like we do it currently and just move the init of that class to an earlier state

Putting that into a static class init would also be a solution, it just loads them when it is needed. I always think they are ugly, but it's a good solution

But we already just use the same method, just renamed it and moved it to the top of the constructor in 1.21 to fix that issue. If there is no particular reason to move to the static init, this should be done for 1.20.4 too.

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

Successfully merging this pull request may close these issues.

3 participants