DELE sub-record handling

[smbas]

It was not so easy. A lot of code was reworked.

Most substantial changes:

• Move DELE handling to the ESM records (mIsDeleted flag). The records that can't be deleted: Game Settings, Skills, Magic Effects.
• Move NAME handling to the ESM records (in some records DELE is appeared before NAME).
• Most records search DELE at every sub-record's position (for example, SNDG record in a deleted form contains only NAME and then DELE in the version 1.20, but in 1.30 it contains all sub-records and DELE at the end).
• Implement saving of deleted records in OpenCS.

All seems to work, but it's needed more testing by someone with a lot of mods.

[zinnschlag]

Error on load in OpenMW-CS: Unknown subrecord. Record: REGN Subrecord DELE Offset: 0x183

Steps to reproduce:

• Create a new addon for Morrowind.esm
• Delete the first region
• Save
• Close
• Load Morrowind and addon into OpenMW-CS

[smbas]

Save format changes:

• Most records: NAME-DELE (end)
• Cells: NAME(if not empty)-DATA-DELE (list of references) (end)
• Infos: INAM-PNAM-NNAM-DELE (end)
• Lands: INTV-DATA-DELE (end)
• LandTextures: NAME-INTV-DATA-DELE (end)
• Pathgrids: NAME-DATA-DELE (end)
• Scripts: SCHD-DELE (end)

[zinnschlag]

Looks okay to me. @scrawl: ?

[ghost]

The naming was changed to "Unknown", and then a comment added that previous naming "Deleted" was actually correct. Why?

[smbas]

I've changed it, because of a deleted flag that was added to the record.

[ghost]

The potential fallout from this PR is huge. I want to do a lot more testing (and maybe some profiling) before merging this.

Speaking of testing, we don't even have ESM tests yet... we should have nice and simple test ESPs for testing things like: merging dialogue infos in the correct order, overwriting records, deleting records, saving a file just like it was read in, etc, etc.

[ghost]

By removing these two lines the overwriting of existing records was broken.

[ghost]

That raises a problem... to merge records in this way, we need the ID of the record, before we load the rest of the record.

Question: do we need merging? It seems like the superior approach, but right now the ESM records save all fields anyway (even non-modified fields). But we might want to expand on this in the future. @zinnschlag: your opinion please?

[zinnschlag]

You mean like only modify a subrecord in an addon and leave the rest of the original record unchanged? Sounds useful, but also very complicated. Definitely not on my list for the near or medium future. OpenMW 2.0-ish? I don't think we need to take that into consideration now.

[smbas]

To be able to merge records we can split the load method into two methods: loadId() and loadData(). But in this case, both methods must have a deleted flag as a parameter. To know whether a record is deleted we must checked them both.

What are your opinions about this, @zinnschlag, @scrawl?

[ghost]

I don't think we need to take that into consideration now.

Right. @smbas: just fix the code to restore the overwriting of records (no merging needed), then we're good.

[ghost]

Question: what happens in MW if content file A deletes a record, then content file B (loaded afterwards) makes a modification to it? Does the record stay deleted?

[kcat]

Can't say for sure about vanilla Morrowind, but I know with Oblivion that causes a crash. When a record gets deleted the engine no longer recognizes it, so trying to do anything to it would try to edit a non-existent object. It led to a tool to mass "undelete and disable" references, because disabled references would still be modifiable thus preventing such crashes, while still being ignored in-game (of course, if something subsequently tries to enable such a reference, it will show up again when it probably shouldn't).

[smbas]

Just tested a few such cases in MW: records don't stay deleted. All modifications are applied.

[ghost]

I'd like to put this PR on hold until I've created the aforementioned unit tests.

[artemutin]

I have tested this branch with my favorite BIG mode, and could report about some issues - referencing a deleted ref (while object ID still remains undeleted) in script by ->disable or ->unlock cause engine to throw an error "Unknown id: someID" and stop script execution. It happens with NPCs and Activators as well. In vanilla MW nothing happens and scripts works well.
Another issue -- spell was deleted from game, but was leaved in Spells list of NPC. As a result i got a warning "Ignoring non-existing spell 'blabla' on NPC 'npcID' "
Also i got some other type of errors, may be it's not related to this branch. I got this on loading Seyda Neen Cell -- error during rendering 'erene llenim': vector::_M_range_check:__n(which is 0)>=this->size()(which is 0)

[ghost]

@artemutin: these problems seem unrelated to the branch in question. Do you get the same problems on the master branch?

Another issue -- spell was deleted from game, but was leaved in Spells list of NPC. As a result i got a warning "Ignoring non-existing spell 'blabla' on NPC 'npcID' "

Working as expected. Using a deleted object should result in a warning.

[artemutin]

1. No, I have not. Master branch works well, but without this mod, obviously.
2. Yep, it should. I understand now, that this is problem with inaccurate scripts and object reference deletion in general in this mod, rather than bug in OpenMW. May be it shouldn't break script execution with exception and just issue a warning, like missing spell reference did. And this kind of errors are candidates to be checked with verifier.

[ghost]

Sorry for the delay in accepting the PR. Didn't want it in 0.37 because it's a big and scary change. I just solved those pesky merge conflicts and will write the unit tests tomorrow. Then we can merge if everything is well.

[psi29a]

+1 for unit tests! :)

[ghost]

Some profiling data:

esm_rewrite:

content files: 191.466 ms

master:

content files: 174.379 ms

margin of error is ~4 ms.

diff --git a/apps/openmw/mwgui/loadingscreen.cpp b/apps/openmw/mwgui/loadingscreen.cpp
index 321d5e6..b0425bb 100644
--- a/apps/openmw/mwgui/loadingscreen.cpp
+++ b/apps/openmw/mwgui/loadingscreen.cpp
@@ -237,6 +237,7 @@ namespace MWGui

     bool LoadingScreen::needToDrawLoadingScreen()
     {
+        return false;
         if ( mTimer.time_m() <= mLastRenderTime + (1.0/mTargetFrameRate) * 1000.0)
             return false;

diff --git a/apps/openmw/mwworld/worldimp.cpp b/apps/openmw/mwworld/worldimp.cpp
index 7e78bef..25eb0ed 100644
--- a/apps/openmw/mwworld/worldimp.cpp
+++ b/apps/openmw/mwworld/worldimp.cpp
@@ -8,6 +8,8 @@
 #include <tr1/unordered_map>
 #endif

+#include <osg/Timer>
+
 #include <osg/Group>
 #include <osg/ComputeBoundsVisitor>
 #include <osg/PositionAttitudeTransform>
@@ -179,8 +181,11 @@ namespace MWWorld
         gameContentLoader.addLoader(".omwaddon", &esmLoader);
         gameContentLoader.addLoader(".project", &esmLoader);

-        loadContentFiles(fileCollections, contentFiles, gameContentLoader);
+        // esm_rewrite: content files: 191.466

+        osg::Timer timer;
+        loadContentFiles(fileCollections, contentFiles, gameContentLoader);
+std::cout << "content files: " << timer.time_m() << std::endl;
         listener->loadingOff();

         // insert records that may not be present in all versions of MW

[ghost]

With Info loading commented out, the difference is much smaller:

esm_rewrite:
content files: 133.964 ms

master:
content files: 128.103 ms

diff --git a/apps/openmw/mwworld/esmstore.cpp b/apps/openmw/mwworld/esmstore.cpp
index 50324f3..e77a591 100644
--- a/apps/openmw/mwworld/esmstore.cpp
+++ b/apps/openmw/mwworld/esmstore.cpp
@@ -71,6 +71,8 @@ void ESMStore::load(ESM::ESMReader &esm, Loading::Listener* listener)

         if (it == mStores.end()) {
             if (n.val == ESM::REC_INFO) {
+                esm.skipRecord();
+                /*
                 if (dialogue)
                 {
                     dialogue->readInfo(esm, esm.getIndex() != 0);
@@ -80,6 +82,7 @@ void ESMStore::load(ESM::ESMReader &esm, Loading::Listener* listener)
                     std::cerr << "error: info record without dialog" << std::endl;
                     esm.skipRecord();
                 }
+                */
             } else if (n.val == ESM::REC_MGEF) {
                 mMagicEffects.load (esm);
             } else if (n.val == ESM::REC_SKIL) {

Which is strange, because nothing really changed in the info loading code as far as I could tell.
Edit: the handling of deleted flag was moved out of the Info record into the info map, so there is slightly more memory used. Well, the performance difference is small enough that it's almost negligible, and if the more consistent deletion handling helps with the OpenCS implementation then I'm ok with that.

[ghost]

Added the first test, all good so far.

$ diff test_dialogue_merging_master.txt test_dialogue_merging.txt
$

[ghost]

#805