Assign constant rather than dynamic blockIndices to modded RMBs #2718
Conversation
AssignNewIndex assigns RMB blocks static BlockIndices from the RMB.json file rather than assigning BlockIndices dynamically
|
Testing this a bit more, I have noticed a bit of a performance hit caused by deserializing lots of large jsons twice (once to get the index and a second time to get the replacement data). I'm hoping that a better coder than I could restructure this to either a) deserialize custom RMBs once, grabbing the index and replacement data all in one go, or b) use the filename method I suggested above. Edit: I did a controlled test and on closer inspection there appears to be no performance hit after all. I guess deserializing a handful of jsons is not that big a deal. |
|
I re-added the original method (AssignNextIndex) as a fallback to ensure backwards compatibility with any mods that don't use unique Index values in their custom RMBs. I don't think this is a big issue, because (as discussed earlier) to the best of my knowledge only Cliffworms, Wayton, and I have added custom RMBs, but this commit will make sure that any others are handled as they were before. |
| jsonBlockIndex = dfBlock.Value.Index; | ||
|
|
||
| // If jsonBlockIndex is invalid (less than or equal to blocksFile.BsaFile.Count), use fallback method | ||
| if (jsonBlockIndex <= blocksFile.BsaFile.Count) |
There was a problem hiding this comment.
blocksFile doesn't exist in the scope and should probably be:
// If jsonBlockIndex is invalid (less than or equal to nextBlockIndex), use fallback method
if (jsonBlockIndex <= nextBlockIndex)
{
AssignNextIndex(blockName);
return;
}There was a problem hiding this comment.
Good catch and a good solution. Thanks, Z-Machine!
|
Solved the double-deserialization issue with the simple and in retrospect obvious fix of caching the DFBlock on the first deserialization. Now GetDFBlockReplacementData will get the DFBlock from the cache rather than deserializing the json the second time. There should no longer be any performance issues with this PR. |
| if (!dfBlock.HasValue) | ||
| { | ||
| Debug.LogError($"Failed to load block data for blockName: {blockName}"); | ||
| throw new System.Exception($"Block {blockName} does not have a valid Index in its JSON file."); |
There was a problem hiding this comment.
If this is gonna throw it should probably have its own exception so it can be caught by the caller. Lines 519 and 524 could also possibly throw invalid cast exceptions, whether or not that can actually happen in the FS library idk.
There was a problem hiding this comment.
I added WorldDataReplacementException and wrapped the (DFBlock)SaveLoadManager.Deserialize(...) calls in a try / catch (InvalidCastException) block.
…n during JSON deserialize
|
|
||
| // Cache the full DFBlock | ||
| if (!blocks.ContainsKey(blockKey)) | ||
| blocks[blockKey] = dfBlock.Value; |
There was a problem hiding this comment.
The cached DFBlock for the AssignNextIndex fallback also needs to have it's internal Index value updated. What I would recommend:
diff --git a/Assets/Scripts/Utility/AssetInjection/WorldDataReplacement.cs b/Assets/Scripts/Utility/AssetInjection/WorldDataReplacement.cs
index 92ddee749..73875e6e4 100644
--- a/Assets/Scripts/Utility/AssetInjection/WorldDataReplacement.cs
+++ b/Assets/Scripts/Utility/AssetInjection/WorldDataReplacement.cs
@@ -627,11 +627,19 @@ namespace DaggerfallWorkshop.Utility.AssetInjection
// If jsonBlockIndex is invalid (less than or equal to nextBlockIndex), use fallback method
if (jsonBlockIndex <= nextBlockIndex)
{
+ int newIndex = nextBlockIndex;
+
AssignNextIndex(blockName);
// Cache the full DFBlock
if (!blocks.ContainsKey(blockKey))
- blocks[blockKey] = dfBlock.Value;
+ {
+ // Update DFBlock index
+ DFBlock block = dfBlock.Value;
+ block.Index = newIndex;
+
+ blocks[blockKey] = block;
+ }
return;
}Otherwise AssignBlockData() will potentially pull the wrong data for RMBs assigned an index through the AssignNextIndex() fallback, causing the player to enter the wrong interior, or throwing an exception because the door.resourceIndex is outside the bounds of the RMB's SubRecords.
This bug is clearly visible here with Cliffworm's release version of Lively Cities.
For more insight I'll use the mod Village of Fishguard by dotWayton to demonstrate the bug step by step.
Here you can see Fishguard's RMBs are both set to index 308 within their JSON files.
And here we can see Fishguard's RMBs have been assigned new indexes by AssignNextIndex():
Attempting to enter a house in FISHER03.RMB results in this:
We can see AssignBlockData() grabbed two RMBs for our location, FISHER02.RMB and FISHER03.RMB (lines 4531 and 4532). And we can immediately see three things are wrong upon it trying to load the record data (on line 4533): it is pulling from FISHER02.RMB instead of FISHER03.RMB, the Block Index and the Door BlockIndex are both wrong (308 instead of 1296), and the Door RecordIndex far exceeds the Block SubRecord length (which causes the exception on line 4534). This is because the internal index value for the block wasn't updated, so the door is still using the old block index of 308, and since FISHER02.RMB is the first block that matches the door's block index of 308 it gets selected.
After the change in the diff above was applied:
It's now pulling from the correct block FISHER03.RMB instead of FISHER02.RMB, with the correct Block Index and Door BlockIndex of 1297 instead of 308, and with the Door RecordIndex correctly within the bounds of the Block SubRecords length.
|
|
||
| // Add to cache | ||
| newBlockNames[jsonBlockIndex] = blockName; | ||
| newBlockIndices[blockName] = jsonBlockIndex; |
There was a problem hiding this comment.
Probably want to add a fallback to AssignNextIndex here too in case there are already entries for the keys jsonBlockIndex and blockName. This way two different modded RMBs with the same custom index (by accident) can still get along.
| newBlockNames[jsonBlockIndex] = blockName; | ||
| newBlockIndices[blockName] = jsonBlockIndex; | ||
| } | ||
|
|
There was a problem hiding this comment.
It's probably better to handle this up above, since in the fallback the new index still has to be updated in the cached DFBlock:
diff --git a/Assets/Scripts/Utility/AssetInjection/WorldDataReplacement.cs b/Assets/Scripts/Utility/AssetInjection/WorldDataReplacement.cs
index 73875e6e4..18e033bcc 100644
--- a/Assets/Scripts/Utility/AssetInjection/WorldDataReplacement.cs
+++ b/Assets/Scripts/Utility/AssetInjection/WorldDataReplacement.cs
@@ -624,8 +624,8 @@ namespace DaggerfallWorkshop.Utility.AssetInjection
// Check for the "Index" field and assign its value
jsonBlockIndex = dfBlock.Value.Index;
- // If jsonBlockIndex is invalid (less than or equal to nextBlockIndex), use fallback method
- if (jsonBlockIndex <= nextBlockIndex)
+ // If jsonBlockIndex is invalid (less than or equal to nextBlockIndex) or index is already taken, use fallback method
+ if (jsonBlockIndex <= nextBlockIndex || newBlockNames.ContainsKey(jsonBlockIndex) || newBlockIndices.ContainsKey(blockName))
{
int newIndex = nextBlockIndex;Other than that, looks good!
This PR is aimed at fixing this rather severe issue with modded RMB files (those with non-vanilla blockNames) first described here: #2703. Because modded RMBs are assigned their blockIndices dynamically rather than deterministically, their blockIndices change when the player restarts the game or loads a save. This breaks all interiors associated with modded RMBs. This means that saves within buildings in modded RMBs are corrupted and cannot be loaded. It also means that questors assigned to buildings in modded RMBs disappear, potentially preventing players from completing quests -- including the main quest. Unfortunately this issue affects some of the most widely used mods, including Fixed Desert Architecture, Finding My Religion, Beautiful Cities/Villages, and Cities Overhauled.
This PR fixes the issue in the simplest possible manner by allowing modders to assign a constant blockIndex to modded RMBs. I replaced the AssignNextIndex method with the AssignNewIndex method, which looks up the Index value from the RMB.json file:
Here AssignNewIndex will always assign this non-vanilla RMB block, TEMPASB0.RMB, the constant index 2012, which is well above the highest vanilla blockIndex (1294).
This PR will require RMB modders to change the Index values in all of their non-vanilla RMBs and to claim unique RMB blockIndices in the Daggerfall Workshop Resource Register. This will not require much adjustment since this is already standard practice for all other assets that require a unique, constant value, like LocationIds, FactionIds, and texture archives. To the best of my knowledge, only Cliffworms, Wayton, and I have made modded new RMBs so far, and coordinating between us will be easy.
I have tested this PR on modded new blocks in Beautiful Cities/Beautiful Villages and can affirm that saves inside modded RMB buildings successfully load and questors appear correctly.
I made this PR as a minimal change. One downside of my approach is that it requires deserializing modded RMB.jsons twice, once to get the Index value in AssignNewIndex and once to get the rest of the data in GetDFBlockReplacementData. While I don't find that this noticeably increases load time at startup, a more adept coder might be able to reorganize the WorldDataReplacements class or BlocksFiles so that it deserializes modded RMB.jsons only once. Alternately, the simplest and least intrusive way might be to move the blockIndex into the RMB.json filename, the way location jsons have their region and regional index in their filename. In this solution TEMPASB0.RMB-2012.json would be the filename for blockIndex 2012, and AssignNewIndex could simply parse the filename instead of deserializing the json. I can implement this if it seems preferable to reviewers.
Finally, please note that I made AssignNewIndex public. This is so that mods (and here I'm particularly thinking of World of Daggerfall) can add new modded RMBs as well. Therefore, this PR replaces 2709.