Record iterators

[Utumno]

We must have a common API for record iterators:

• getActiveRecords must go - getActiveRecords is just fine for now but we must:
  • decide on deleted/ignored records - are we including those in scanModFile? add comments -> turns out a full new issue: #653
  • return the record id and not the fid - this will open the way for absorbing ImportCells and similar as we won't be doing the ignored (see initData anc co)
• indexRecords must go (started in 2e3e5ee1e26c8892f459944a53e5c03c3a6d106c)
  • MobDial should be simpler to patch
  • MobObjects is the goal - we have this keyedByEid stuff...
• iterators should return the key also - first thing we do is record.fid after the for loop
• and of course make a proper Mob classes hierarchy that is game agnostic - this is a bit tougher

Branch: https://github.com/wrye-bash/wrye-bash/tree/utumno-647

Blocks #480, #312

[Infernio]

@Utumno Is it a problem that signatures for records that the game doesn't have end up in RecordType.sig_to_class? I randomly noticed this in the BashBugDump while validating record definitions in FO4, so added a quick deprint:

--- a/Mopy/bash/game/patch_game.py
+++ b/Mopy/bash/game/patch_game.py
@@ -276,3 +276,5 @@ def _validate_records(cls, package_name, plugin_form_vers=None):
         # that's the case for most games so do it here and override if needed
         brec.RecordType.simpleTypes = set(cls.top_groups) - {b'CELL', b'WRLD',
                                                              b'DIAL'}
+        from ..bolt import deprint
+        deprint(brec.RecordType.sig_to_class)

This gives the following output for Morrowind:

patch_game.py  280 _validate_records: {b'FLST': <class 'bash.brec.common_records.AMreFlst'>, b'IMAD': <class 'bash.brec.common_records.AMreImad'>, b'CELL': <class 'bash.game.morrowind.records.MreCell'>, b'WRLD': <class 'bash.brec.common_records.AMreWrld'>, b'ASTP': <class 'bash.brec.common_records.MreAstp'>, b'COLL': <class 'bash.brec.common_records.MreColl'>, b'DEBR': <class 'bash.brec.common_records.MreDebr'>, b'DLBR': <class 'bash.brec.common_records.MreDlbr'>, b'DLVW': <class 'bash.brec.common_records.MreDlvw'>, b'DUAL': <class 'bash.brec.common_records.MreDual'>, b'EYES': <class 'bash.brec.common_records.MreEyes'>, b'FSTP': <class 'bash.brec.common_records.MreFstp'>, b'FSTS': <class 'bash.brec.common_records.MreFsts'>, b'GLOB': <class 'bash.game.morrowind.records.MreGlob'>, b'GMST': <class 'bash.brec.common_records.MreGmst'>, b'TES3': <class 'bash.game.morrowind.records.MreTes3'>, b'ACTI': <class 'bash.game.morrowind.records.MreActi'>, b'ALCH': <class 'bash.game.morrowind.records.MreAlch'>, b'APPA': <class 'bash.game.morrowind.records.MreAppa'>, b'ARMO': <class 'bash.game.morrowind.records.MreArmo'>, b'BODY': <class 'bash.game.morrowind.records.MreBody'>, b'BOOK': <class 'bash.game.morrowind.records.MreBook'>, b'BSGN': <class 'bash.game.morrowind.records.MreBsgn'>, b'CLAS': <class 'bash.game.morrowind.records.MreClas'>, b'CLOT': <class 'bash.game.morrowind.records.MreClot'>, b'CONT': <class 'bash.game.morrowind.records.MreCont'>, b'CREA': <class 'bash.game.morrowind.records.MreCrea'>, b'DIAL': <class 'bash.game.morrowind.records.MreDial'>, b'DOOR': <class 'bash.game.morrowind.records.MreDoor'>, b'ENCH': <class 'bash.game.morrowind.records.MreEnch'>, b'FACT': <class 'bash.game.morrowind.records.MreFact'>, b'INFO': <class 'bash.game.morrowind.records.MreInfo'>, b'INGR': <class 'bash.game.morrowind.records.MreIngr'>, b'LAND': <class 'bash.game.morrowind.records.MreLand'>, b'LEVC': <class 'bash.game.morrowind.records.MreLevc'>, b'LEVI': <class 'bash.game.morrowind.records.MreLevi'>, b'LIGH': <class 'bash.game.morrowind.records.MreLigh'>, b'LOCK': <class 'bash.game.morrowind.records.MreLock'>, b'LTEX': <class 'bash.game.morrowind.records.MreLtex'>, b'MGEF': <class 'bash.game.morrowind.records.MreMgef'>, b'MISC': <class 'bash.game.morrowind.records.MreMisc'>, b'NPC_': <class 'bash.game.morrowind.records.MreNpc_'>, b'PGRD': <class 'bash.game.morrowind.records.MrePgrd'>, b'PROB': <class 'bash.game.morrowind.records.MreProb'>, b'RACE': <class 'bash.game.morrowind.records.MreRace'>, b'REGN': <class 'bash.game.morrowind.records.MreRegn'>, b'REPA': <class 'bash.game.morrowind.records.MreRepa'>, b'SCPT': <class 'bash.game.morrowind.records.MreScpt'>, b'SKIL': <class 'bash.game.morrowind.records.MreSkil'>, b'SNDG': <class 'bash.game.morrowind.records.MreSndg'>, b'SOUN': <class 'bash.game.morrowind.records.MreSoun'>, b'SPEL': <class 'bash.game.morrowind.records.MreSpel'>, b'SSCR': <class 'bash.game.morrowind.records.MreSscr'>, b'STAT': <class 'bash.game.morrowind.records.MreStat'>, b'WEAP': <class 'bash.game.morrowind.records.MreWeap'>}

Note a bunch of record types showing up here that Morrowind doesn't have, e.g. b'ASTP': <class 'bash.brec.common_records.MreAstp'> (which exists since Skyrim).

[Utumno]

Yes this is due to the classes in common records - it does not harm but I was thinking of chopping those off based on the valid_header_sigs and adding the missing valid_header_sigs (for instance the ref records for skyrim) to the sig_to_class dict as MreRecord - that way we get rid of the valid record sigs in favor of sig_to_class - I think this will work but I start to grok all the interactions just now :)

EDIT: valid_header_sigs not valid_record_sigs - I'' bin them :P

[Infernio]

Reported on Discord:

patch_files.py  266 scanLoadMods: MERGE/SCAN ERROR: Oblivion Citadel Door Fix.esp
Traceback (most recent call last):
  File "bash\patcher\patch_files.py", line 251, in scanLoadMods
  File "bash\patcher\patch_files.py", line 283, in mergeModFile
  File "bash\brec\record_groups.py", line 114, in merge_records
bash.exception.AbstractError: <bash.brec.record_groups.MobBase object at 0x0000025216F7EB90>: merge_records not implemented

[Utumno]

Do we have a reproducer?

[Infernio]

Oblivion Citadel Door Fix.esp, should come with UOP

[Infernio]

Reported on Discord:

patcher_dialog.py  272 _error: Exception during Bashed Patch building:
Traceback (most recent call last):
  File "bash\basher\patcher_dialog.py", line 184, in PatchExecute
  File "bash\basher\patcher_dialog.py", line 281, in _save_pbash
  File "bash\mod_files.py", line 257, in safeSave
  File "bash\mod_files.py", line 291, in save
  File "bash\brec\record_groups.py", line 278, in dump
  File "bash\brec\record_groups.py", line 529, in dump
  File "bash\brec\record_groups.py", line 945, in dump
  File "bash\brec\record_groups.py", line 527, in dump
  File "bash\brec\record_groups.py", line 529, in dump
  File "bash\brec\record_groups.py", line 944, in dump
  File "bash\brec\record_groups.py", line 235, in _write_header
  File "bash\brec\mod_io.py", line 152, in pack_head
  File "bash\brec\mod_io.py", line 201, in _pack_args
  File "bash\brec\utils_constants.py", line 290, in dump
bash.exception.AbstractError: Dumping a dummy fid

[Infernio]

My test BP now matches dev 100% (except for the fact that it fixes the record count, which is broken on dev). I tested merging of simple FLOR records, DIAL records with their children and all kinds of CELL and WLRD children combinations. Will test a full-LO BP next.

[Infernio]

Nothing corrupt, but the keepRecords/updateRecords stuff isn't working right for CELL/WRLD groups because I end up with hundreds of CELL/WRLD ITMs.

Edit: anyways, I'm calling it for today. I probably won't have time to look at this for the next few days (what with Christmas and all 😅).

[Utumno]

Maybe time for _process_rec to go there too

Edit: merry christmas Inf 🎄 🤶 (and if you have any local 647 changes don't forget to push)

On Fri, Dec 23, 2022, 11:39 PM Infernio @.***> wrote:

Nothing corrupt, but the keepRecords/updateRecords stuff isn't working right for CELL/WRLD groups because I end up with hundreds of CELL/WRLD ITMs.

— Reply to this email directly, view it on GitHub https://github.com/wrye-bash/wrye-bash/issues/647#issuecomment-1364349904, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKNIV5BPZU2AY2OJB4BMALWOYLX7ANCNFSM6AAAAAAQ3QJM7A . You are receiving this because you were mentioned.Message ID: @.***>

[Utumno]

Just to say I added the Oblivion default removals and some more code for 647 ( the diff is https://github.com/wrye-bash/wrye-bash/compare/a720da221e8722c8767000c4704c1f4aa1eda03c...a2ed5ca6a32e812d1a775383b76c38695f1ad675 ) on nightly - I believe both are safe but with 647 we can't be sure - so if we have any regressions have a look there. It's basically splitting getNumRecords into two methods and adding a get_num_headers to MreRecord so we get rid of a couple overrides of Mob.get_num_headers - also seems the empty_mob overrides were not needed

[Infernio]

@Utumno The regression with mergeability reported on Discord is because iter_present_records only returns the highest level records when not given a signature now. However, the usage in isPBashMergeable wants all records in the hierarchy to determine if they are new records or not.

I have worked around this for now like so:

-        for rfid, _rec in block.iter_present_records(): # skip deleted/ignored
-            if rfid.mod_fn == self_name:
+        for candidate_rec in block.iter_records():
+            if candidate_rec.should_skip(): continue # skip deleted/ignored
+            if candidate_rec.group_key().mod_fn == self_name:

But this is obviously a bad solution, we're trying to centralize all that should_skip stuff. Maybe split iter_present_records into two methods, one for the top-level records only and one for all in the hierarchy, with an optional signature filter?

[Utumno]

Thanks for the fixups Inf - the keep stuff was a bit half baked and I needed a second pair of eyes but I think it's good - apart from the fact that when we pass a complex record it should check its master record. Another thing that stopped me was the Mob hierarchy itself - I want to ideally get rid of setChanged there altogether - use MobBase when we read the group as a blob and the rest for unpacking (the check != MobBase in ModFile.load is a hint). But then one realizes that the iter_record stuff is all Abstract really for MobBase - so all this is fluid. Let me think a bit more - for now one little should_skip is not that bad, given we already encapsulated most of the handling. One possible solution would be to do the should_skip in iter_records - I will rescan the code and maybe come up with something. Meanwhile if you can spend some brain power the ITPOs are a good candidate :)

[Infernio]

If by ITPOs you mean the extra WRLDs, those should be fixed as of bb9ca60b2914711250630bf4a74bc44c3059c56f. That might not be the right way to fix it though.

[Utumno]

Probably, as for the one right way - well it's still fluid - will have a closer look right now.

Then another one (and arguably trickier as it needs a dive in c1711dc9982ffaca0d78dd9803293a204b3d7b3f) - from 9850e015c68a2623171ffd702bca0a065598ddb7:

Also dropped _validate, would have complicated this even further plus I seriously doubt ROAD behaves any different from PGRD and LAND - Beth love reusing code for stuff like this.

Well I am afraid the comment meant the opposite - validate should be kept if ROAD behaves the same. But all the validation of PGRD/LAND went bye bye in c1711dc9982ffaca0d78dd9803293a204b3d7b3f so we may need to reinstate it for feature parity (or rather clear superiority 🧐 ) of 647 over previous code.

I won't touch TempRefs for now so if you have some time...

(EDIT: it's not exactly error checking but the fact that we have special attributes (pgrd, land, road) means that at least we match the record type - now TempRefs being a MobObjects never looks at the record type - now having a record with the same fid changing record type (so be PGRD in one plugin and REFR in another) might not really be worth checking - dunno)

[Infernio]

Wasn't the original validation so that if you have two mods adding new PGRDs (or LANDs) in the same CELL's temp children group with different FormIDs, we raise an error? And we had to drop that because it's actually well defined behavior by the game that some mods rely on (the later-loading mod's PGRD/LAND wins)?

Then "ROAD behaves the same" means we don't want to raise an error for it either, but instead allow the later-loading mod to override, even if the FormIDs differ.

Edit: see c9ca0468116a893c88f81f6d54e3030ef9325cb3.

[Utumno]

Edit: see https://github.com/wrye-bash/wrye-bash/commit/c9ca0468116a893c88f81f6d54e3030ef9325cb3.

Now I understand what that was about - so this settles, no validation (yey!) - but in that case shouldn't we now (on merge) check record signatures rather than fids - we might end up with two PGRD in the same temp children group? That was what the old code was doing - the new one I think just picks formids, doesn't ever override via rec_sig information

Edit: working on it - turns out needs quite a few work

[Infernio]

Yeah, two PGRDs should never be allowed. If one plugin adds a PGRD to a CELL, then another adds one to that same CELL then no matter the FormIDs, the second one will replace the first one. Same goes for LAND and (probably) for ROAD.

[Utumno]

Seems the current code could do that, right? Edit: did some refactoring in merge_records to make it easy to mimic the behavior of _Nested for TempRefs - f72b78b43019faf71d8781aabc76307f993f65f6

[Infernio]

Reported on Discord:

patch_files.py  329 scanLoadMods: MERGE/SCAN ERROR: Immersive Interiors Fixes.esp
Traceback (most recent call last):
  File "bash\patcher\patch_files.py", line 312, in scanLoadMods
  File "bash\patcher\patch_files.py", line 344, in mergeModFile
  File "bash\brec\record_groups.py", line 332, in merge_records
  File "bash\brec\record_groups.py", line 141, in _has_missing_masters
AttributeError: 'dict' object has no attribute 'issuperset'

Mod in question: https://www.nexusmods.com/oblivion/mods/51589