search

Interkarma / daggerfall-unity

Open source recreation of Daggerfall in the Unity engine
http://www.dfworkshop.net
MIT License
2.7k stars 327 forks source link

City generation stability improvements #2617

Open KABoissonneault opened 5 months ago

KABoissonneault commented 5 months ago

Throwing an exception during city generation generally has a massive effect on the player. Often, the entire city disappears, and if not, huge blocks are missing.

This PR tackles two city generation exceptions, with the intention of preventing one bad model from destroying the entire city. Instead, we just skip bad objects (but we keep errors in the logs).

Bad model id

For the first issue, when a modded custom RMB tries to spawn a model (exterior block 3d object record) that doesn't exist, we get this exception

NullReferenceException: Object reference not set to an instance of an object
DaggerfallWorkshop.Utility.ModelCombiner.Add (DaggerfallWorkshop.ModelData& modelData, UnityEngine.Matrix4x4 matrix) (at Assets/Scripts/Utility/ModelCombiner.cs:137)
DaggerfallWorkshop.Utility.RMBLayout.AddModels (DaggerfallWorkshop.DaggerfallUnity dfUnity, ...)
DaggerfallWorkshop.Utility.RMBLayout.CreateBaseGameObject (DaggerfallConnect.DFBlock& blockData, ...) DaggerfallWorkshop.Utility.GameObjectHelper.CreateRMBBlockGameObject (DaggerfallConnect.DFBlock blockData, ....)

In RMBLayout.AddModels, we call dfUnity.MeshReader.GetModelData(obj.ModelIdNum, out modelData) with the provided model id num. but we don't check whether the call succeeded or not. If it fails, all the data in modelData will be null. This is fine if MeshReplacement.ImportCustomGameobject(obj.ModelIdNum, parent, modelMatrix) succeeds on the model id, but if not, we treat it like a classic model, except we don't actually have the data. Hence the NullReferenceException above.

I reproduced this by playing with Fixed Desert Architecture and an outdated "Daggerfall Expanded Textures" (the models FDA was using was in 0.6.3, I was on 0.5).

To fix this, I track whether importing the classic data succeeded. If we fail at importing a custom model and we failed at getting the classic data, then I log the error and skip the object.

Weird error in StaticNPC.SetLayoutData

I got two different callstacks from users where an entire city disappeared after a null access in StaticNPC.SetLayoutData(DFBlock.RmbBlockFlatObjectRecord obj, int mapId, int locationIndex). This function has three versions, one for dungeons, one for buildings, and one for "custom": this is modded static NPCs that exist outside in the city. This is called by RMBLayout in two functions: AddMiscBlockFlats and AddExteriorBlockFlats. We've had the same error occur in from both functions, looking like this.

NullReferenceException: Object reference not set to an instance of an object
  at DaggerfallWorkshop.Game.StaticNPC.SetLayoutData (DaggerfallConnect.DFBlock+RmbBlockFlatObjectRecord obj, System.Int32 mapId, System.Int32 locationIndex) [0x0004e] in <52ba1c52c60a4e479fd500dae43b0a6f>:0 
  at DaggerfallWorkshop.Utility.RMBLayout.AddExteriorBlockFlats (DaggerfallConnect.DFBlock& blockData, UnityEngine.Transform flatsParent, UnityEngine.Transform lightsParent, System.Int32 mapId, System.Int32 locationIndex, DaggerfallWorkshop.ClimateNatureSets climateNature, DaggerfallWorkshop.ClimateSeason climateSeason) [0x00194] in <52ba1c52c60a4e479fd500dae43b0a6f>:0

SetLayoutData is not a big function. I've looked at it for at least an hour, and I'm confident that the only things that could be null in there are DaggerfallUnity.Instance or its ContentReader or its FlatsFileReader, or flatCFG.gender. I've ruled out the first three, because both AddMiscBlockFlats and AddExteriorBlockFlats check that DaggerfallUnity.Instance is not null and ready before calling this code. So it has to be the FLATS.CFG data that has a null in it somehow. I have no idea how it happens, maybe there's a mod out there that uses reflection to slip its own custom data in there.

So I'm doing two things for this issue:

  1. Added a null check for the flatCFG.gender in StaticNPC.SetLayoutData
  2. Added a try-catch around this code

The change looks big, but it's really just indenting everything for the try-catch

If I failed to catch the real exception, then at least we avoid destroying the entire city. Generally, I would rather know where the bad data is and how it occurs, but I've never been able to reproduce it. However, two separate users got it with different setups, so I'm sure it's a real issue.

The error message in the logs looks like

Error creating exterior block flat obj '2952' of block 'ARMRAL02.RMB' 

NullReferenceException: Object reference not set to an instance of an object
  at DaggerfallWorkshop.Game.StaticNPC.SetLayoutData (DaggerfallConnect.DFBlock+RmbBlockFlatObjectRecord obj, System.Int32 mapId, System.Int32 locationIndex) [0x0004e] in <52ba1c52c60a4e479fd500dae43b0a6f>:0 
  at DaggerfallWorkshop.Utility.RMBLayout.AddExteriorBlockFlats (DaggerfallConnect.DFBlock& blockData, UnityEngine.Transform flatsParent, UnityEngine.Transform lightsParent, System.Int32 mapId, System.Int32 locationIndex, DaggerfallWorkshop.ClimateNatureSets climateNature, DaggerfallWorkshop.ClimateSeason climateSeason) [0x00194] in <52ba1c52c60a4e479fd500dae43b0a6f>:0 
UnityEngine.DebugLogHandler:Internal_LogException(Exception, Object)
UnityEngine.DebugLogHandler:LogException(Exception, Object)
UnityEngine.Logger:LogException(Exception, Object)
UnityEngine.Debug:LogException(Exception)
DaggerfallWorkshop.Utility.RMBLayout:AddExteriorBlockFlats(DFBlock&, Transform, Transform, Int32, Int32, ClimateNatureSets, ClimateSeason)
DaggerfallWorkshop.Utility.GameObjectHelper:CreateRMBBlockGameObject(DFBlock, Int32, Int32, Int32, Int32, Boolean, DaggerfallRMBBlock, DaggerfallBillboardBatch, DaggerfallBillboardBatch, DaggerfallBillboardBatch, TextureAtlasBuilder, DaggerfallBillboardBatch, ClimateNatureSets, ClimateSeason)
DaggerfallWorkshop.<UpdateLocation>d__81:MoveNext()
UnityEngine.SetupCoroutine:InvokeMoveNext(IEnumerator, IntPtr)
UnityEngine.MonoBehaviour:StartCoroutineManaged2(IEnumerator)
UnityEngine.MonoBehaviour:StartCoroutine(IEnumerator)
DaggerfallWorkshop.StreamingWorld:InitPlayerTerrain()
DaggerfallWorkshop.StreamingWorld:Update()
ajrb commented 5 months ago

I've always thought the reason for these errors not to be handled more gracefully was so it was very obvious an issue has occured. Players aren't checking logs for exceptions and wont report issues unless it impacts them in a way that can't be ignored. Since we're post 1.0 this may not be desired anymore since it's a poor user experience, but without issues and mod conflicts being obvious I suspect issues will lurk in the shadows a lot more. I am not sure what the best course is, but I think it' worth a discussion..

KABoissonneault commented 5 months ago

I've always thought the reason for these errors not to be handled more gracefully was so it was very obvious an issue has occured. Players aren't checking logs for exceptions and wont report issues unless it impacts them in a way that can't be ignored.

Nowadays, lots of players run mods like Exception Monitor and Retro Frames that report exceptions during gameplay as they happen. Honestly, I'm thinking DFU could have an option for something similar built-in. This is better to me than letting the exception run through and do something random as the report mechanism.

For example, between the two issues I have here, one issue is a mod configuration failure (didn't have the right DET for FDA), while the other is a completely unknown issue that still needs to be determined. In both cases the user gets a disappearing city. Their only recourse is to go ask support what their problem is, because the only thing the logs mention is "NullReferenceException".

This is a burden for support. At the very least, the logs should be better at mentioning which object in which block causes the issue.

I am, in addition, taking the decision here to just remove specific the specific objects rather than stopping the player's gameplay. If the objects are important, people will report it, and the logs will be more useful than before this change. This is a part I'm still willing to discuss, but I really think we need a better error reporting mechanism instead.

PS: This is not a v1.1 issue, so we got some time to decide our strategy for v1.2

CabraArgento commented 5 months ago

Nowadays, lots of players run mods like Exception Monitor and Retro Frames that report exceptions during gameplay as they happen. Honestly, I'm thinking DFU could have an option for something similar built-in.

An "off/non-intrusive warning symbol on the corner (click to open player.log?)/big Exception Monitor style warning in the middle of the screen" slider would work wonders, especially if the default setting is the non-intrusive warning symbol.

Players who hate it for some reason can disable it, players with massive modlists and experimental mod/game builds can set it to the big warning and for those who never notice it, any screenshot they share will have a blatant "ask this person for their player.log" warning for anyone in the know.

KABoissonneault commented 5 months ago

Despite my initial claims, I'm now hesitating to merge this until we have better error reporting to players. My primary concern was the gender issue, but we found it was Roleplay&Realism causing this, and we released a fix. I'm leaving this on hold until v1.2