AdvancedPeripherals icon indicating copy to clipboard operation
AdvancedPeripherals copied to clipboard

Fixed NullPointerException on startup

Open philipp-gatzka opened this issue 8 months ago • 5 comments

PLEASE READ THE GUIDELINES BEFORE MAKING A CONTRIBUTION

  • Please check if the PR fulfills these requirements
  • [x] The commit message are well described
  • [x] 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
  • [x] 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:

philipp-gatzka avatar Apr 25 '25 13:04 philipp-gatzka

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

zyxkad avatar Apr 25 '25 14:04 zyxkad

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

philipp-gatzka avatar Apr 25 '25 14:04 philipp-gatzka

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

zyxkad avatar Apr 25 '25 15:04 zyxkad

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

philipp-gatzka avatar Apr 25 '25 15:04 philipp-gatzka

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.

SirEndii avatar Apr 25 '25 19:04 SirEndii