[WIP] Mutable ESM records (#2798)

[Assumeru]

Since rot provided a nice list, I'll just copy the things that I'm sure need doing.

• actor record, when using AI parameter functions like SetFight
• actor records, when using AddSpell, RemoveSpell
• script records, when editing local variables with "scriptID".localvar
• AddItem for containers
• AddItem for non-unique actors

Levelled lists have been in for years and I haven't checked how faction reactions and weather are handled... but I think those also work? Leaving script records, when editing local variables with "scriptID".localvar. I know that syntax works, but does it work like it's supposed to?

[Capostrophic]

does it work like it's supposed to?

no

[Assumeru]

Adding it to the list then.

[Capostrophic]

You may need to revise container behavior too, see UESP notes on AddItem.

[Assumeru]

And actor behaviour, looking at that. Beth sure came up with some interesting functionality...

[akortunov]

Also maybe it is worth to implement different tasks in different PR's. For example, containers are not really related to AddSpell or SetFight.

[Assumeru]

Yeah, splitting between actor and non-actor stuff might be wise.

[psi29a]

Just a comment... if you are going to be touching the for loops, could you perhaps clean them up ta bit?

For example, you could pull this up into one readable line.

for (std::vector<std::string>::const_iterator iter (ref->mBase->mSpells.mList.begin());
    iter!=ref->mBase->mSpells.mList.end(); ++iter)

for (auto iter (ref->mBase->mSpells.mList.begin()); iter!=ref->mBase->mSpells.mList.end(); ++iter)

[psi29a]

Just a something I noticed that you can really clean up the readability of the code you are already touching anyway. Take advantage of modern C++ ;D

[psi29a]

gcc wasn't happy about that...

CMakeFiles/openmw_test_suite.dir/__/openmw/mwworld/esmstore.cpp.o: In function `void __gnu_cxx::new_allocator<MWMechanics::SpellList>::construct<MWMechanics::SpellList, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int&>(MWMechanics::SpellList*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int&)':
esmstore.cpp:(.text._ZN9__gnu_cxx13new_allocatorIN11MWMechanics9SpellListEE9constructIS2_JRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERiEEEvPT_DpOT0_[_ZN9__gnu_cxx13new_allocatorIN11MWMechanics9SpellListEE9constructIS2_JRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERiEEEvPT_DpOT0_]+0x60): undefined reference to `MWMechanics::SpellList::SpellList(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int)'
collect2: error: ld returned 1 exit status

[Assumeru]

Right... so that's a pretty ridiculous problem. Can't the unit tests just pull in the apps/openmw cmake list so I don't have to manually tell it to build every (transitively) included cpp file?

[elsid]

Run CMake with BUILD_UNITTESTS=ON.

[Assumeru]

That's really not what I mean. Anyway, sleepy time, g'night.

[elsid]

Probably you mean to setup build targets properly. For now there is no library build target including ../openmw/mwmechanics/spelllist.cpp, only binary includes this cpp: https://github.com/OpenMW/openmw/blob/master/apps/openmw/CMakeLists.txt#L105 . And it's not possible to link binary to binary. So we need to build a library and then link it to tests binary. Correct me if I'm wrong but I don't know other way to setup dependencies.

[Assumeru]

I think that's what I want, but I know just about nothing about CMake. The way unit tests are built strikes me as pretty fragile at the moment - with the upside of reduced build time, of course.

[psi29a]

/home/travis/build/OpenMW/openmw/apps/openmw_test_suite/mwworld/test_store.cpp: In constructor ‘MWMechanics::SpellList::SpellList(const string&, int)’:
/home/travis/build/OpenMW/openmw/apps/openmw_test_suite/mwworld/test_store.cpp:14:83: error: class ‘MWMechanics::SpellList’ does not have any field named ‘mSpellsChanged’
   14 |     SpellList::SpellList(const std::string& id, int type) : mId(id), mType(type), mSpellsChanged(false) {}
      |                                                                                   ^~~~~~~~~~~~~~

and

/home/travis/build/OpenMW/openmw/apps/openmw/mwmechanics/spells.cpp:435:13: warning: add explicit braces to avoid dangling else [-Wdangling-else]
3169            else
3170            ^
31711 warning generated.

[Assumeru]

Nearly there I think. Still kinda need to sync actors with the base record as I don't think instances can have their own spell lists at all.

[psi29a]

How does this correspond to your list of check boxes in the OP?

[Assumeru]

The current changes are working towards the second checkbox.

[psi29a]

/home/travis/build/OpenMW/openmw/apps/openmw_test_suite/mwworld/test_store.cpp:14:57: error: invalid use of incomplete type ‘class MWMechanics::SpellList’
   14 |     SpellList::SpellList(const std::string& id, int type) : mId(id), mType(type) {}
      |                                                         ^
In file included from /home/travis/build/OpenMW/openmw/apps/openmw_test_suite/mwworld/test_store.cpp:10:
/home/travis/build/OpenMW/openmw/./apps/openmw/mwworld/esmstore.hpp:18:11: note: forward declaration of ‘class MWMechanics::SpellList’
   18 |     class SpellList;
      |           ^~~~~~~~~

[psi29a]

radioactive ;)

[Assumeru]

radioactive ;)

But can we have Capo sing the song for the release trailer? :D

[psi29a]

I do not remember merging this explicitly... I did merge #2980 though.

[Assumeru]

I guess it's technically correct since this PR is a subset of the other one at the moment. If it doesn't reopen automatically when I get started on the last item, I'll just make a new one.

[Assumeru]

Container notes

Explicit Add/Remove item does not appear to affect the base recordAnother test found no difference, I may have emptied the explicit target in my first test.
Adding/removing items from containers gives them inventory data; taking everything adds a 1 byte MNAM subrecord with value 0
Implicit Add/Remove item affects the base record
Implicit Additem affects containers with custom non-empty inventories in the active cell (cells probably)
Implicit add/remove item does not work on empty containers

[akortunov]

Explicit Add/Remove item does not appear to affect the base record

If I remembered correctly, there is an another system. In Morrowind all containers or actors with the same ID use shared inventory, so Additem/RemoveItem affects all instances, including newly-spawned ones. It is also possible to add levelled items here.
In some cases (e.g. when player takes an item from container) object can get an unique inventory, so Add/Remove on this object affect only target instance of object. It is possible to add levelled lists here, but they are not replaced to real items (may be fixed by MCP).

Just select object in console. If there is only object ID, then object should not have its own inventory. If there is an eight digit suffix (e.g. "hul00000000"), object has an unique inventory.

Also such "shared" inventories seem to be resolved temporary in Morrowind - of you open a container without taking any items and use save/load, there will be different items. Once you take an item from container, its content becomes fixed and restock stops to work for it.

In OpenMW once you open a container, it get a permanent resolved inventory, but restocking still works. It leads to exploits, also it makes merchant items in OpenMW less random than in Morrowind:

1. Items, resolved for merchant, are permanent and are not restocked until you buy them.
2. You can steal all items from restocking container, then open barter menu for victim trader. Looted container will be fully restocked.

[Assumeru]

This is what the MCP has to say:

AddItem with levelled items

Adds support for levelled items to AddItem. The target reference for AddItem should be a reference to a clone, not a base object.

The game stores the levelled lists in a base object, and cloning the object applies the levelled randomization. You can identify if a reference is cloned or not by selecting it with the console. A base object has a plain id (foronir), while a clone has an eight digit hexadecimal suffix (foronir00000000).

For containers, the clone happens when the player looks inside a container, and importantly, the container is de-cloned when it is empty. Adding anything to a base container will affect every container of that type, therefore you have to be careful adding levelled items to container references. Use a unique container to avoid this problem altogether.

The impression I was getting was that the shared inventory is just a temporary view of the base record (i.e., levelled lists are resolved) that only gets saved once a change is made to it.

The note on merchants is interesting. I'll definitely need to do some testing in that regard. Also negative quantities are always pretty odd.