Dynamic Surroundings

Dynamic Surroundings

51M Downloads

Conflict with WorldDownloader 1.12.2

Tsoccerguy3 opened this issue ยท 11 comments

commented

Mod Version:
1.12.2

Forge Version:
b2775

Link to crash log (if applicable):

Getting a similar crash with WorldDownloader conflict , the logs on my OneDrive http://sdrv.ms/TPcx5v in ore directory

commented

This appears to be entirely different than the other crash. I wish there was more detail as to what was going on in the log. DS is no where in the stack traces. :\

commented

Wait a sec - does this operate on the client thread? If it is trying to save the player entity on the client side I think I Know what the problem is.

commented

Yes, it does try to save the player entity on the client side. I'm 90% sure it's from the client thread, but some of the code is pretty old and it might be doing some unsafe threading stuff (I've been meaning to audit code for thread-safety for a while but haven't fully gotten around to it).

(I'm not 100% sure how soccerguy narrowed it down to this mod specifically; however, from testing, I do know that the crash happens in the middle of vanilla NBT code writing forge capabilities based off of a partially written file)

commented

That's exactly the issue. With v3.5 of DS I create a capability on the client player entity (not server side) that has no persistance. The serializer for the capability returns null, not expecting the client to want to serialize the player info. I am going to make a change to return back an empty compound tag which should address the issue.

commented

Thanks, makes sense. I'll look into sending a patch to forge to make some kind of warning if a null NBT value is set so that issues like this fail during the serialization to NBT (including the tag name and such) and not during the writing to disk part (without including the tag) to make stuff like this easier to debug in the future.

commented

Problem Fixed in Dev , working with no conflict , Thank You

commented

Nice - I forgot that you do builds :)

commented

Looking at it, it's unclear whether returning null is supposed to be allowed; there is a forge issue (MinecraftForge/MinecraftForge#5197) related to it. But at least currently, it does seem to cause crashes, so returning an empty value makes sense for now at least.

commented

My first reaction is that there should be non-persistent capabilities. But then that would not make sense (from a generalized perspective) if the caps need to be sent the client, whether as part of the client/server handshake or data sync. What may be more of interest is purely client side or purely server side caps. I think the current design assumes that if a cap exists on one side it must exist on the other.

commented

I've submitted a simple forge patch to fail earlier when null is used in setTag, though it doesn't address the more general things with null and capabilities (I still need to read more on the capabilities system in general).

commented

Version v3.5.0.1 BETA pushed to CurseForge.