SpongePowered / SpongeForge

A Forge mod that implements SpongeAPI
http://www.spongepowered.org/
MIT License
1.14k stars 306 forks source link

UniDict Registry ordering issue #1897

Closed D3nnis3n closed 6 years ago

D3nnis3n commented 6 years ago

I am currently running

Issue Description After i updated to the first sponge Version built against Forge 2555 i get the issue that (if i understood the developer of Base/Modern/*Metals correctly) the IDs for CrackHammer Recipes are not the same on Server and Client on every restart despite the configs being EXACTLY the same. This leads to the recipes being missing and wanted to be removed on Server Startup on every Server Startup by forge despite nothing has changed between Startups like so:

Missing mmdlib:crusher_registry: basemetals:oreiolite_to_iolite_powder basemetals:oreadamantine_to_adamantine_powder basemetals:ingotcadmium_to_cadmium_powder basemetals:ingotvioletsapphire_to_violetsapphire_powder basemetals:ingotlithium_to_lithium_powder basemetals:ingottanzanite_to_tanzanite_powder basemetals:nuggetgalvanizedsteel_to_galvanizedsteel_smallpowder basemetals:cobblestone_wall_to_gravel basemetals:orezirconium_to_zirconium_powder basemetals:stone_to_cobblestone basemetals:cobblestone_to_gravel [...]

This also leads to a fatally missing registry error when trying to join the Server. As soon as i remove sponge neither the missing registry error above nor the fatally missing registry error on join occur anymore.

[05:28:34] [Netty Client IO #1/ERROR] [FML]: basemetals:nether_zirconium_ore_to_zirconium_ore: 457
[05:28:34] [Netty Client IO #1/ERROR] [FML]: basemetals:ingotcharcoal_to_charcoal_powder: 269
[05:28:34] [Netty Client IO #1/ERROR] [FML]: basemetals:nuggetboron_to_boron_smallpowder: 347
[05:28:34] [Netty Client IO #1/ERROR] [FML]: Network Disconnect: Fatally missing registry entries
[05:28:34] [Netty Client IO #1/FATAL] [FML]: Failed to connect to server: there are 246 missing registry items

The developer in discord said the following:

DShadowWolf - letzten Montag um 12:11 Uhr frown that is the crackhammer recipes not getting identical id-numbers on client and server due to minor differences in reg order Shouldn't be an issue, but Spongeforge makes it one

Thiakil - gestern um 08:46 Uhr How exactly do you guys add stuff to your registry? using the event? DShadowWolf - gestern um 08:59 Uhr no DShadowWolf - gestern um 09:00 Uhr has to happen long before Forge would fire the event well, for the parts that don't bomb out we could do the crusher/crackhammer at any point thing is... we've got a registry that is two or three times the size that just works anyway... its 0300 and I need to head to bed

As of this playing with Sponge is no longer possible on the versions made for forge 2555. I also noticed the dev, so he might post here. You can also contact him on the MMD OreSpawn Discord in #metals_minerals_gems

Otherwise i'll Forward your opinion on this to him, if you think it's not on your side.

phit commented 6 years ago

Add some logs and some way to reproduce this for someone that has no idea how that mod works. Right now I'm just seeing quotes that don't make much sense to me, assuming you are meaning block/item ids? Aren't those synced to the client when they join a server? They haven't been static since like 1.6.4.

D3nnis3n commented 6 years ago

Which logs do you want me to provide exactly? I've shown you the relevant parts that Show the Problem.

Which Client logs, which Server logs? Complete Startup?

The Modder says "recipe hammer recipes don't get the same ids on Server and Client with sponge installed".

You can reproduce it by installing this mod: https://minecraft.curseforge.com/projects/base-metals And sponge. On every sponge Startup the Server then want's to remove missing recipes (shown above) again. And you can't connect as of an fatally missing registry entry.

D3nnis3n commented 6 years ago

Okay, i made two starts that are exactly the same mods and configs, first time without sponge - everything works finde, then with sponge and i was ordered to acknowledge 337 "missing" registry entries on start.

Here you get the complete logs of my log Folder: http://minecraft.godsofwar.de/server_without_sponge.tgz http://minecraft.godsofwar.de/server_with_sponge.tgz

I hope that helps. When sponge is active on the Server in this constellation i cannot join as Client because of "fatally missing registry" - if sponge is not active, i can join without Problems.

If you Need anything more than this, please ask me to provide exactly what you Need - i'm no dev, i don't know. But i will provide anything you want as Long as it's there.

ryantheleach commented 6 years ago

DShadowWolf - gestern um 09:00 Uhr has to happen long before Forge would fire the event well, for the parts that don't bomb out

I'm no expert at Forge modding, but this statement outright scares me.

The registry events were made for a reason, and from what I've seen Lex is pretty adamant about people using it.

The registry events also give a very clearly defined point in the lifecycle where Sponge can expect the registrations to be finished, so any assumptions that Sponge might be making about when things are registered are completely off the table if the mod isn't using the registry events...

That said, this could still be on us to fix, There's been a report on the forums about the block registry not matching as well.

You say you suspect that it's related to our compatibility with Forge 2555 Would you be able to see if you can replicate this with 1.12.2-2555-7.0.0-BETA-2756 Which is the first version that was deemed compatible with 2555?

If your intuition is correct this should help narrow down the range of versions, narrowing it down further would really be appreciated.

D3nnis3n commented 6 years ago

Yes, it occurs also with this Version - i updated to the latest sponge from that Version today, as i wanted to check if the issue has already been fixed. It must have been a Change in the Version that was build for the forge versions you choose before 2555. I'll try a bit around for you. (At least if my "Intuition" is correct)

As for ShadowWolf: He is a very skilled developer in my opinion and if he says he Needs to do it before the Events are fired (his mod is big and does so many Things with compatibility to so many others mods) i do believe that. But i sent your text over to him, as i'm no developer - only sysad.

D3nnis3n commented 6 years ago

My suspecton was wrong. The mod causing this issue seems to be UniDict. With Version 1.3 they added "basemetals" Support for the crusher recipes. And as soon as i use a unidict Version below this Version, Startup and Connection with sponge is no longer an issue. Seems like this mod Version has a problem with sponge after updating to 1.3 or higher.

I opened an issue over there. So the problem is more with unidict then with basemetals. But the problem is their added compatibility for basemetals.

https://github.com/WanionCane/UniDict/issues/60

I hope this helps sorting the problem out. For now i can use a older UniDict Version to Play again.

D3nnis3n commented 6 years ago

I can now also say the following: When you add sponge it will tell you that unidict:recipes are missing. When you remove sponge and Startup again it tells you basemetals:recipes are missing. When you then start again it works like it should with no missing recipes and everyone can connect.

Funny. (At least for me who has no idiea what is actually Happening)

D3nnis3n commented 6 years ago

Maybe unidict does not Register their altered recipes at the same time when basemetals does that and this is only a problem with sponge installed? I dunno. It's funnny at least. If you guys Need any further Information from me, just ask, i'll be happy to provide what i can.

dshadowwolf commented 6 years ago

Okay, looks like UniDict might be accessing and modifying the "Crusher Recipe Registry" badly. As said to @D3nnis3n on Discord, *Metals runs all of its internal registry stuff (which the "Crusher" registry certainly counts as) a long time before Forge ever fires the related events. This is because, mostly, those registries are used for things like "holding the instance of a block or item that is to be registered with forge"

Now... the "CrusherRecipeRegistry" is created as an "IForgeModifiableRegistry" and is made available via a simple API... For what UniDict is doing it should be as simple as "com.mcmoddev.lib.registry.CrusherRecipeRegistry.getAll()" with recipes removable by several exported methods that provide convenience wrappers around the actual IForgeModifiableRegistry.remove() method.

EDIT: After checking out the UniDict source code it seems that the issue is that UniDict only does non-core registry modification client-side. In this case it leads to the "CrusherRecipeRegistry" on the server having all of its entries while on the client it has been modified. This is a worrying thing, to me, and has been reported to the UniDict developers.

dshadowwolf commented 6 years ago

@ryantheleach at any point up until Forge actually "locks" the registries you can access them - even long before the registry events themselves fire. However... BaseMetals has a set of "not IForgeRegistry" registries it uses for tracking a lot of stuff - the "Crusher Recipe Registry" itself has been, recently, changed in when it gets manipulated and another change is going to come because its needed.

As to my earlier statement... Turns out that the code is a bit more complex in Unidict than I'd seen and what I was seeing as only client-side was Recipe Book edits. There are a couple places where FML events (among others) are used to actually give timing for editing the various registries it does. So at this point there is little known information about what is triggering this.

(As I've said to @Meronat and @gabizou on the MMD Discord, identical configs on server and client should result in identical results)

D3nnis3n commented 6 years ago

The issue still persists without disabling the integration. The Modmaker thinks "wontfix". Anything you can do about this?

Zidane commented 6 years ago

From reading the above, there is some shady crap going on with the registries in Unidict and I suspect that Sponge is only showing the issue and its quite likely this is happening regardless. We literally do little to the registry system in Forge...the extent of which is simply to synchronize what is in the Forge registry for blocks/items/etc to what is available to our plugins.

We can't fix what is very likely bad code in another mod.