Forge Config API Port

Forge Config API Port

29M Downloads

[Bug]: Mod shadows nightconfig instead of JiJing, causing potential issues with mods that JiJ a different version

lukebemish opened this issue ยท 17 comments

commented

Mod Loader (Required)

Quilt

Minecraft Version (Required)

1.20.1

Mod Version (Required)

8.0.0

Notes (Required)

I have a mod that JiJs nightconfig. I cannot guarantee that I will always keep the same version, for the same minecraft version, as forgeconfigapiport. This leads to two issues:

  • If I JiJ an older version of nightconfig than you, and you depend on new features; if the classes from my JiJed version get picked, there is an issue
  • If I JiJ a newer version of nightconfig than you, and depend on new features; if the classes from your shadowed version get loaded instead of the JiJed ones, there is an issue

Is there any particuar reason you are shadowing nightconfig instead of JiJing it, or at the absolute least relocating it? If nothing else, it creates a large amount of log spam while quilt warns the user about duplicate classes being present.

latest.log (Optional)

No response

commented

This may have been caused by the fix to #42. Is it possible to relocate it if you're going to shadow it or is it supposed to be exposed to end users?

commented

(Notably, I still suspect the thing that made #42 hard to solve was caused by some sort of fabric loader bug, so figuring out a consistent, simple reproduction case for that bug and reporting it to fabric loader, and getting a fix in on that end, is probably the better solution in the long run, as the "intended" solution for stuff like this is JiJing, so if that isn't working something has gone wrong)

commented

Relocating is not an option unfortunately as that breaks multi-loader projects, since Night Config classes will no longer be the same as on Forge.

I see your point and I thought about that, too, but honestly as long as this remains just a potential issue I'm fine with the current approach of shadowing the library. Night Config has only been receiving very small bugfix updates in the recent years and it doesn't look like that's going to change anytime soon.

Maybe I'll check out JiJ again for some future release and do some testing regarding the issue it was giving me in the 1.18 era, but yeah as long as there is no actual issue here I don't see the need to act right now.

commented

If JiJ was giving you issues, that points towards a loader bug, so we should probably look into that and try to pin it down. Shadowing is... Not a long term solution, due to the log spam that ensues if nothing else.

commented

I'm not 100% sure but it seems to break Distant Horizons 2.0.0' config:
https://mclo.gs/jEs3DiS

commented

This seems unrelated, the TOML writer simply doesn't support maps.

I was able to reproduce this exact issue with a simple Night Config instance just by setting up a toml config with a map value and trying to write that to a file.

commented

Ah, alright. I haven't done great tests for this, but DH's config breaks due to some mod compat. Still not sure what mod then, it's not Cloth Config or YACL.
Sorry for the bother!

commented

Ay awesome, glad you guys came to a conclusion.

commented

Distant Horizons dev here.
I did some digging and was able to reproduce the issue with the following:

  • DistantHorizons-2.0.0-a-dev-1.20.1
  • fabric-api-0.86.1+1.20.1
  • ForgeConfigAPIPort-v8.0.0-1.20.1-Fabric
  • LegendaryTooltips-1.20.1-fabic-1.4.4
    • Iceberg-1.20.1-fabric-1.1.13 needed by LegendaryTooltips
    • Prism-1.20.1-fabric-1.0.5 needed by LegendaryTooltips

It appears the issue is that both ForgeConfig and DH shade electronwill.nightconfig in an incompatible way.

Here's what I think is happening:

  1. ForgeConfig loads, putting its version of nightconfig into the JVM
  2. A user opens and closes DH's config
  3. DH tries to use its version of nightconfig to save the config
  4. Since the nightconfig versions don't match (DH's version supports LinkedHashMap's, it appears ForgeConfig's does not), DH fails to save its config and (intentionally) crashes the game.

I'll see if I can fix this on DH's side, but if anyone else wants to use a shaded version of electronwill.nightconfig, this could be an issue.

commented

It doesn't look to me like a version mismatch could be an issue here, both our mods shade v3.6.6 of Night Config.
Also I don't see how the two shades would interfere since they are placed in different domains, but not sure about that and what else besides the domains could matter.
Also could you please provide a compiled jar of your latest Fabric release so I can do some testing on my own?

commented

Oh, we are using the same version? That's odd...
DH can use HashMap<>'s in our config if ForgeConfig (and other mods) aren't present. I'll have to do some more digging.

Here is the jar I've been using for testing:
DistantHorizons-fabric-2.0.0-a-dev-1.20.1.zip

To reproduce the issue:

  1. Download the mods listed in my post above.
  2. Launch MC Fabric 1.20.1
  3. Open MC's options menu
  4. Click on DH's config icon next to the FOV slider
  5. Click the "advanced" button
  6. The game should crash
commented

Oh this is weird...
When I run DH by itself our HashMap<string, string> config entry's value is automatically converted to a string in nightconfig.TableWriter, but when I have ForgeConfig, the config entry returns the HashMap<> object instead.

Is there some setting in nightconfig that would cause this behavior (unfortunately I wasn't the dev who set up nightconfig initially, so I'll have to do some digging to see what setup DH is doing).

commented

Oh, well this would've saved us all a lot of effort.
NightConfig doesn't support HashMap<>'s: TheElectronWill/night-config#114 so DH has just been using NightConfig wrong.

I'll fix DH so it doesn't use HashMap<>'s (we can just (de)serialize them into JSON and have NightConfig handle that); however, its very weird that this issue only popped up when both DH and ForgeConfig were running together.

commented

Yeah that's what I meant here haha: #45 (comment)
It's also stated here: https://github.com/TheElectronWill/night-config/wiki/Config-formats#files

commented

Haha, yeah, my bad. sorry for the wild goose chase and thanks for the help.

commented

Fixed in commit fc54e0f8 on our GitLab.

commented

Decided to give JiJ another try as of v20.4.1, let's see how it goes.. ๐Ÿ‘€