Rework FormIDs handling

[Utumno]

Moving forward in refactoring records (namely removing the setDefault API) convertFids stands in the way. Apart from the obvious inelegance of the current design (as we should do the "conversion" the moment we load, as we are well aware we have a formid at hand, rather than in bulk afterwards), in MelUnion and co we have no way of knowing if we have a fid till we load (which is one of the main failures of setdefault the other being - what default value exactly?). Add to this that we don't really need to convert anything apart from when comparing/hashing etc - so in a laudable addition to the python Zen:

Just in time is better than just in case

The downside is some inevitable performance loss as we have a class instead of builtin tuples/ints - but this refactoring will permit us thinning the MelBase/MobBase (record groups) APIs and eventually drop setDefault, so we will gain the performance back and more.

Blocks #480 and #525

[Infernio]

Started looking into the 637 issues, tried building a tiny patch (in Enderal) to start with. Was curious why it only had three records in it (according to Details), loaded it in xEdit:

Whoops, all records got a NULL FormID 😄

[Infernio]

Okay, fixed that one - it was a rogue walrus operator: 02780fe82bc005f64bcda7641fc255593f5783cb Will have a look at the rest tomorrow.

[Utumno]

Thanks Inf :)

For my part, I fixed the issue I had that made me worry about my sanity - d71063e462770042c1aab25e61fe27b0a48485bc - so silly once fixed, due to the 'DATA' I included looking at the Oblivion TES4 (...) - we were probably working at it at the same time - I just had no www :P

With that, I think the branch is almost nightly ready

[Infernio]

Looks great so far 👍

[Utumno]

Thanks Inf!

Pushed a rebased version -> 265b8564881040386e7af8130c53d0c2fd584b15 - started smoothing out and addressed your comments (like HITMEs and co) - apart those should be as 477c28378624fb7303026087a5c2df987abd8d5c - needs some more passes - what really is over me is write some get-me-started tests :P - granted what really needs testing is beyond the scope of this merge (loading plugins...)

[Infernio]

Getting an error in Oblivion that I'm not sure how to solve:

patcher_dialog.py  265 _error: Exception during Bashed Patch building:
Traceback (most recent call last):
  File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\utils_constants.py", line 140, in __ne__
    return self.long_fid != other.long_fid
AttributeError: 'int' object has no attribute 'long_fid'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\basher\patcher_dialog.py", line 170, in PatchExecute
    patchFile.buildPatch(log,SubProgress(progress,0.8,0.9))#no speeding needed/really possible (less than 1/4 second even with large LO)
  File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\patcher\patch_files.py", line 322, in buildPatch
    self.tes4.masters = self.getMastersUsed()
  File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\mod_files.py", line 326, in getMastersUsed
    block.updateMasters(masters_set.add)
  File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\record_groups.py", line 270, in updateMasters
    record.updateMasters(masterset_add)
  File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\record_structs.py", line 605, in updateMasters
    element.mapFids(self, masterset_add)
  File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\advanced_elements.py", line 922, in mapFids
    element.mapFids(record, function, save_fids)
  File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\basic_elements.py", line 803, in mapFids
    if any((rec_val := getattr(record, at)) is not None and
  File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\basic_elements.py", line 804, in <genexpr>
    rec_val != dflt for at, dflt in zip(self.attrs, self.defaults)):
  File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\utils_constants.py", line 147, in __ne__
    raise StateError(f'Comparing {self!r} with {other}')
bash.exception.StateError: Comparing _Tes4Fid((Oblivion.esm, 000000)) with 0

The record in question is (Knights.esm, 002D77) a PACK. The subrecord is PTDT. That one is in a union:

MelUnion({
    (0, 1): MelOptStruct(b'PTDT', [u'i', u'I', u'i'], u'targetType',
        (FID, u'targetId'), u'targetCount'),
    2: MelOptStruct(b'PTDT', [u'i', u'I', u'i'], u'targetType', u'targetId',
        u'targetCount'),
}, decider=PartialLoadDecider(
    loader=MelSInt32(b'PTDT', u'targetType'),
    decider=AttrValDecider(u'targetType'),
)),

You can see cases 0 and 1 are FormIDs, but case 2 is an int. So it seems to end up as an int here, but for some reason MelOptStruct tries to compare it against the FormID anyways?

Edit: getting the same error in Skyrim too, probably the same subrecord too (that part of PACK hasn't changed much).

[Infernio]

Re: performance - I did some quick profiling (see my two latest commits on the branch, which are the results of that). Times are really inconsistent, as per usual with IO-reliant profiling. So afterwards I did five tests (well, 5x2 - five on dev, five on 637) without profiling active:

FNV, dev: =====================================================================
0:00:27.359
0:00:33.047
0:00:31.672
0:00:31.5
0:00:34.938

Average: 0:00:31.604
Median: 0:00:31.672

FNV, utumno-637-fids: =========================================================
0:00:33.859
0:00:37.625
0:00:35.234
0:00:36.594
0:00:30.688

Median: 0:00:35.234
Average: 0:00:34.800

637 does seem somewhat slower, but I have no idea where exactly it comes from yet - could just be the extra overhead of creating FormId objects, could be something else as well. The only thing I can say so far is that there are definitely fewer function calls. The reason seems to be that FormIds are immutable and hence deepcopy can skip them, whereas before it had to deepcopy the tuples.

[Utumno]

Re PTDT: Attempted a fix in 901980c5b76f469798f7b1667a3bd0a630cfb7eb

Re: timing - yes function calls went down a lot (think convertFids) but I think the extra overhead comes from using __equals__ and co in python land instead of the tuples one that should be much faster. Nothing much to be done - bear in mind I already threw some opts in - maybe margin for more but we will get the performance back binning setDeafult (and we might rewrite some of it in assembly a la xEdit :D)

[Infernio]

Pushed a rebased version to xxx-utumno-637-fids, there wasn't much in the way of conflicts :)

The fix seems to work, BP completes in Oblivion now 👍 I'll check if the BPs are identical in all games, and if so then I think we can stick it on nightly.

[Infernio]

There are some BP differences in Oblivion (one weird empty extra GMST is dumped and some LVLIs aren't getting items merged(?) in for some reason), I'll take a look at those.

[Utumno]

Was about to push a rebase myself - started squashing and cleaning up

[Infernio]

Okay, the problem is that convertFids used to call setChanged(): https://github.com/wrye-bash/wrye-bash/blob/00c9ee8298ca78c522e8ab85611348a3c6ead8e3/Mopy/bash/brec/record_structs.py#L226

Without that call, almost all records (besides some random ones in the leveled lists patcher) are marked as unchanged and so just dump out the data that was initially loaded. Not sure how to fix this - maybe drop the whole changed/unchanged distinction on a record level? That might make writing too slow though?

[Utumno]

Ooops - interesting bit of information :D

Hmmm can't tell - this means that all records that were loaded (according to the loadFactory) basically would setChanged to True. Maybe add this to the keep functionality of the BP?

[Infernio]

That would work for the BP - do we modify in any other places (e.g. links) without explicitly calling setChanged on the altered records?

[Utumno]

That's following ModFile.load I guess (tough - although there was quite a bit of centralizing) - we might want to substitute the places that convertFids was called with a lighter one that calls setChanged on all records

[Infernio]

Yeah, that might be the best solution for now. We can always toss that method in some later refactoring once we've hunted load down further.

[Utumno]

If you don't do it I will tomorrowish - now I got the rebase blues 🎶

[Utumno]

Another thing to check is saves a bit especially renaming the Player / importing face. Thanks Inf, I couldn't pull it out alone here :)

[Infernio]

Note for the future: if we change masters, then doing something like a recursive setChanged will still be necessary, since changing masters could impact every single FormID in the plugin we're trying to write.

[Infernio]

Okay, that's fixed the biggest problem, but there are still some weird differences between the BPs. So, investigation time again...

[Infernio]

Turns out it they just needed a setChanged in both the load and dump positions where convertFids used to be. Oblivion BP is now identical, will take a look at the other games.

[Infernio]

Bashed Patches are all identical now, merged it to nightly.

[Utumno]

A million thanks - will have a look and smooth out a bit for future dev merge - probably we don't need the xxx-rebase branch?

[Infernio]

Already gone :)

[Infernio]

Question: with the new API, what's the intended way to check if a FormID isn't NULL (i.e. 0 as a short fid, (game_master, 0) as a long fid)? I'm doing if fid.short_fid: right now, which seems to work? But I guess that might be equal to the object index for long FormIDs? Haven't really dug through that part yet :P

Edit: oh, I guess it's if fid != ZERO_FID:?

[Utumno]

Now it does depend a bit on context - if fid != ZERO_FID would fail if fid was loaded with a ShortFidContext.

[Infernio]

Hmm. NVNM has a component that works as follows:

• First, load a FormID.
• If that FormID is not NULL, load two signed 16-bit integers.
• If it is NULL, load another FormID.

In the previous system, I can handle that as follows:

• On load: Load the FormID, check if it's 0 (since all I did was unpack an int - no long-mapping has happened yet).
• On dump: The FormID has already been short-mapped, so I can check against 0 again.

Right now I changed it to check against ZERO_FID. Maybe FormIDs should have a is_null method that encapsulates this?

Edit: that would also let us get rid of the bush import in FidNotNullDecider.

[Utumno]

ZERO_FID permits us to trace the places we use such a null fid - that should be the exception. I would not add a method just yet - let's keep it explicit - uses are few (and better stay so - as you saw I replaced a few with DummyFid so we don't accidentally dump placeholder null fids - keep an eye on uses of ZERO_FID)

Edit: that would also let us get rid of the bush import in FidNotNullDecider.

what a joy :)

[Infernio]

First 637 report:

Traceback (most recent call last):
  File "bash\gui\events.py", line 178, in _post
  File "bash\balt.py", line 815, in _conversation_wrapper
  File "bash\basher\patcher_dialog.py", line 170, in PatchExecute
  File "bash\patcher\patch_files.py", line 322, in buildPatch
  File "bash\mod_files.py", line 332, in getMastersUsed
  File "bash\brec\record_groups.py", line 279, in updateMasters
  File "bash\brec\record_structs.py", line 605, in updateMasters
  File "bash\brec\basic_elements.py", line 434, in mapFids
  File "bash\brec\basic_elements.py", line 434, in mapFids
  File "bash\brec\complex_subrecords.py", line 206, in mapFids
  File "bash\brec\complex_subrecords.py", line 144, in mapFids
  File "bash\mod_files.py", line 43, in add
AttributeError: 'int' object has no attribute 'mod_id'

[Infernio]

That's probably due to the NVNM thing I fixed actually, let me push a nightly with that.

Edit: actually, it looks like it's CTDA. Asked for a BashBugDump.

Edit 2: fixed in 268ea58a88ea94a861e89286999d10718d7e56f6.

[Infernio]

Here's a tough one:

• In Skyrim (LE or SE, doesn't matter), duplicate Update.esm.
• Allow editing of masters.
• Change its Skyrim.esm master to something else, e.g. Dawnguard.esm.

This gives a traceback like this:

record_structs.py  200 dumpData: Error dumping data: 
Traceback (most recent call last):
  File "bash\brec\record_structs.py", line 198, in dumpData
  File "bash\brec\basic_elements.py", line 202, in dumpData
  File "bash\brec\advanced_elements.py", line 423, in pack_subrecord_data
  File "bash\brec\advanced_elements.py", line 457, in _pack_array_data
  File "bash\brec\basic_elements.py", line 775, in packer
TypeError: 'NoneType' object is not callable

Because there's no write context for FormIDs, but we did load some (the ONAM in Update.esm's TES4 record). So I added a write context to ModInfo.writeHeader, but that leads to a new traceback:

record_structs.py  200 dumpData: Error dumping data: 
Traceback (most recent call last):
  File "bash\brec\record_structs.py", line 198, in dumpData
  File "bash\brec\basic_elements.py", line 202, in dumpData
  File "bash\brec\advanced_elements.py", line 423, in pack_subrecord_data
  File "bash\brec\advanced_elements.py", line 457, in _pack_array_data
  File "bash\brec\basic_elements.py", line 770, in packer
  File "bash\brec\mod_io.py", line 405, in _short_mapper
KeyError: FName('Skyrim.esm')

And now I'm stuck. That master is gone, but all the FormIDs still try to look it up. I guess we'd need to pass the pre-reassignment masters into the write context somehow?

[Utumno]

Oops missed that will have a look when back in a couple hours

On Thu, Aug 4, 2022 at 6:08 PM Infernio @.***> wrote:

Here's a tough one:

• In Skyrim (LE or SE, doesn't matter), duplicate Update.esm.
• Allow editing of masters.
• Change its Skyrim.esm master to something else, e.g. Dawnguard.esm.

This gives a traceback like this:

record_structs.py 200 dumpData: Error dumping data: Traceback (most recent call last): File "bash\brec\record_structs.py", line 198, in dumpData File "bash\brec\basic_elements.py", line 202, in dumpData File "bash\brec\advanced_elements.py", line 423, in pack_subrecord_data File "bash\brec\advanced_elements.py", line 457, in _pack_array_data File "bash\brec\basic_elements.py", line 775, in packer TypeError: 'NoneType' object is not callable

Because there's no write context for FormIDs, but we did load some (the ONAM in Update.esm's TES4 record). So I added a write context to ModInfo.writeHeader, but that leads to a new traceback:

record_structs.py 200 dumpData: Error dumping data: Traceback (most recent call last): File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\record_structs.py", line 198, in dumpData element.dumpData(record,out) File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\basic_elements.py", line 202, in dumpData value = self.pack_subrecord_data(record) File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\advanced_elements.py", line 423, in pack_subrecord_data sub_data += self._pack_array_data(array_val) File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\advanced_elements.py", line 457, in _pack_array_data return b''.join(map(self._element.packer, array_val)) File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\basic_elements.py", line 770, in packer return super().packer(utils_constants.short_mapper(form_id)) File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\mod_io.py", line 405, in _short_mapper return ((object_id := formid.object_dex) >= 0x800 and indexes[ KeyError: FName('Skyrim.esm')

And now I'm stuck. That master is gone, but all the FormIDs still try to look it up. I guess we'd need to pass the pre-reassignment masters into the write context somehow?

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

[Utumno]

First thing tomorrow but meanwhile - if an ONAM FormId refers to Skyrim.esm then if we change the master to anything else this FormId is anyway invalid (or am I missing something?)

But yes in this case the solution is to pass a MasterMap=None kind of thing to the __init__ of the context and catch the KeyError and remap the mod_id (mod_fn) to the new master. I think it's the "change master" functionality that needs revisiting though

On Thu, Aug 4, 2022 at 11:47 PM Mr&Mrs D @.***> wrote:

Oops missed that will have a look when back in a couple hours

On Thu, Aug 4, 2022 at 6:08 PM Infernio @.***> wrote:

Here's a tough one:

• In Skyrim (LE or SE, doesn't matter), duplicate Update.esm.
• Allow editing of masters.
• Change its Skyrim.esm master to something else, e.g. Dawnguard.esm.

This gives a traceback like this:

record_structs.py 200 dumpData: Error dumping data: Traceback (most recent call last): File "bash\brec\record_structs.py", line 198, in dumpData File "bash\brec\basic_elements.py", line 202, in dumpData File "bash\brec\advanced_elements.py", line 423, in pack_subrecord_data File "bash\brec\advanced_elements.py", line 457, in _pack_array_data File "bash\brec\basic_elements.py", line 775, in packer TypeError: 'NoneType' object is not callable

Because there's no write context for FormIDs, but we did load some (the ONAM in Update.esm's TES4 record). So I added a write context to ModInfo.writeHeader, but that leads to a new traceback:

record_structs.py 200 dumpData: Error dumping data: Traceback (most recent call last): File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\record_structs.py", line 198, in dumpData element.dumpData(record,out) File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\basic_elements.py", line 202, in dumpData value = self.pack_subrecord_data(record) File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\advanced_elements.py", line 423, in pack_subrecord_data sub_data += self._pack_array_data(array_val) File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\advanced_elements.py", line 457, in _pack_array_data return b''.join(map(self._element.packer, array_val)) File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\basic_elements.py", line 770, in packer return super().packer(utils_constants.short_mapper(form_id)) File "E:\Infernio\Desktop\Programming\wrye-bash\Mopy\bash\brec\mod_io.py", line 405, in _short_mapper return ((object_id := formid.object_dex) >= 0x800 and indexes[ KeyError: FName('Skyrim.esm')

And now I'm stuck. That master is gone, but all the FormIDs still try to look it up. I guess we'd need to pass the pre-reassignment masters into the write context somehow?

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

[Infernio]

if an ONAM FormId refers to Skyrim.esm then if we change the master to anything else this FormId is anyway invalid (or am I missing something?)

The point of Change Masters is to change a master's name without changing FormIDs at all. This is useful e.g. when a plugin has been renamed, but still has the same internal structure. For example, USLEEP -> USSEP, or renaming the CC .esl files to .esm files so they work in the VR games (which is where this came up in the first place). It's a decently commonly used feature, since the only other way to do it is to use xEdit with two advanced command line arguments.

Honestly I think best way to handle this would be to make a load context that skips doing anything to FormIDs - just leaves them as ints. Then we can change the masters and the FormIDs will correctly resolve to the newly changed masters (or not - this is absolutely functionality that lets users shoot themselves in the foot, which is why it's behind an activation prompt and warning).

[Utumno]

Oh I see now - then this is ShortFidWriteContext in combination with ModReader itself which does not do any translation

[Infernio]

Tried doing that just now. The problem I now run into is that to make that happen, I have to always read headers without FormID translation - which notably means that our ONAMs will no longer be FormIDs but instead just ints, and that breaks Add/Remove ESM Flag, which uses update_onam and so assigns proper FormIDs before writing.

So maybe a 'master map' mechanism is needed after all.

Edit: I think I've got something decent, will test.

[Infernio]

Yup, works great. See f0c608b3ee6482c2145655ce2241c4551b4d2149.

[Utumno]

Thanks Inf for fixups (and 543 ones) - next time I will be in the neighborhood of a computer for a couple hours will merge this on dev

[Infernio]

Pushed everything I had on nightly pre-637 to dev to save you having to merge those too.

[Infernio]

Another regression:

record_structs.py  200 dumpData: Error dumping data: 
Traceback (most recent call last):
  File "bash\brec\record_structs.py", line 198, in dumpData
  File "bash\brec\advanced_elements.py", line 940, in dumpData
  File "bash\brec\basic_elements.py", line 202, in dumpData
  File "bash\brec\basic_elements.py", line 611, in pack_subrecord_data
  File "bash\brec\utils_constants.py", line 247, in dump
bash.exception.AbstractError: Dumping a dummy fid

record_structs.py  201 dumpData: Occurred while dumping <AccompanySergiusSu20x4 [PACK:(Oblivion.esm, 01FC72)]>
record_structs.py  211 dumpData: > time: 20
record_structs.py  211 dumpData: > aiType: 7
record_structs.py  211 dumpData: > eid: 'AccompanySergiusSu20x4'
record_structs.py  211 dumpData: > flags: 0x00000001 (offersServices)
record_structs.py  211 dumpData: > targetId: _FormID((Oblivion.esm, 01D209))
record_structs.py  211 dumpData: > _union_type_21: 0
record_structs.py  211 dumpData: > duration: 4
record_structs.py  211 dumpData: > unused1: b'\x00\x00\x00'
record_structs.py  211 dumpData: > locId: _DummyFid((Oblivion.esm, 000000))
record_structs.py  211 dumpData: > targetCount: 100
record_structs.py  211 dumpData: > targetType: 0
record_structs.py  211 dumpData: > locRadius: 0
record_structs.py  211 dumpData: > date: 0
record_structs.py  211 dumpData: > conditions: []
record_structs.py  211 dumpData: > locType: 0
record_structs.py  211 dumpData: > day: 0
record_structs.py  211 dumpData: > month: -1
patcher_dialog.py  265 _error: Exception during Bashed Patch building:
Traceback (most recent call last):
  File "bash\basher\patcher_dialog.py", line 183, in PatchExecute
  File "bash\basher\patcher_dialog.py", line 274, in _save_pbash
  File "bash\mod_files.py", line 288, in safeSave
  File "bash\mod_files.py", line 322, in save
  File "bash\brec\record_groups.py", line 265, in dump
  File "bash\brec\record_groups.py", line 254, in getSize
  File "bash\brec\record_groups.py", line 255, in <genexpr>
  File "bash\brec\record_structs.py", line 450, in getSize
  File "bash\brec\record_structs.py", line 595, in dumpData
  File "bash\brec\record_structs.py", line 198, in dumpData
  File "bash\brec\advanced_elements.py", line 940, in dumpData
  File "bash\brec\basic_elements.py", line 202, in dumpData
  File "bash\brec\basic_elements.py", line 611, in pack_subrecord_data
  File "bash\brec\utils_constants.py", line 247, in dump
bash.exception.AbstractError: Dumping a dummy fid

Happens because of this PACK:

You can see it's lacking the PLDT, so we dump it out. But since it got setDefaulted, it's a dummy FID and errors on dump. I think we could fix this by making the struct a MelOptStruct, but that seems like it's going to turn into another game of whack-a-mole where we get reports of struct X causing dumping errors and we have to just run around the codebase marking things as MelOptStruct. Plus that has another problem: MelOptStruct skips dumping entirely if the values - even if loaded from a previous plugin - match the defaults. Which could break other subrecords when we do it.

Really, the proper solution is the two-tier system: mark elements as required or optional. Required elements always have to be dumped, optional ones only if they were present when loaded. This subrecord, for example, is not required, so we don't have to dump it. Then we can use MelOptStruct for the handful of cases where we really do want to skip dumping entirely if values match the defaults (though I can't think of any places where we actually want to do that... - if we do, it'll become a three-tier system). But then that's way out of scope for now - so what do for the moment?

[Utumno]

The DummyFid I introduced partly to probe the setDefault path - we can change it to a zeroFid for now and focus in uprooting the setDefault. Then we should rethink optional as you describe - so in this case we should ideally write nothing since we loaded nothing. Will have a closer look once near a computer

On Sat, Aug 13, 2022, 12:39 PM Infernio @.***> wrote:

Another regression:

record_structs.py 200 dumpData: Error dumping data: Traceback (most recent call last): File "bash\brec\record_structs.py", line 198, in dumpData File "bash\brec\advanced_elements.py", line 940, in dumpData File "bash\brec\basic_elements.py", line 202, in dumpData File "bash\brec\basic_elements.py", line 611, in pack_subrecord_data File "bash\brec\utils_constants.py", line 247, in dump bash.exception.AbstractError: Dumping a dummy fid

record_structs.py 201 dumpData: Occurred while dumping <AccompanySergiusSu20x4 [PACK:(Oblivion.esm, 01FC72)]> record_structs.py 211 dumpData: > time: 20 record_structs.py 211 dumpData: > aiType: 7 record_structs.py 211 dumpData: > eid: 'AccompanySergiusSu20x4' record_structs.py 211 dumpData: > flags: 0x00000001 (offersServices) record_structs.py 211 dumpData: > targetId: _FormID((Oblivion.esm, 01D209)) record_structs.py 211 dumpData: > _union_type_21: 0 record_structs.py 211 dumpData: > duration: 4 record_structs.py 211 dumpData: > unused1: b'\x00\x00\x00' record_structs.py 211 dumpData: > locId: _DummyFid((Oblivion.esm, 000000)) record_structs.py 211 dumpData: > targetCount: 100 record_structs.py 211 dumpData: > targetType: 0 record_structs.py 211 dumpData: > locRadius: 0 record_structs.py 211 dumpData: > date: 0 record_structs.py 211 dumpData: > conditions: [] record_structs.py 211 dumpData: > locType: 0 record_structs.py 211 dumpData: > day: 0 record_structs.py 211 dumpData: > month: -1 patcher_dialog.py 265 _error: Exception during Bashed Patch building: Traceback (most recent call last): File "bash\basher\patcher_dialog.py", line 183, in PatchExecute File "bash\basher\patcher_dialog.py", line 274, in _save_pbash File "bash\mod_files.py", line 288, in safeSave File "bash\mod_files.py", line 322, in save File "bash\brec\record_groups.py", line 265, in dump File "bash\brec\record_groups.py", line 254, in getSize File "bash\brec\record_groups.py", line 255, in File "bash\brec\record_structs.py", line 450, in getSize File "bash\brec\record_structs.py", line 595, in dumpData File "bash\brec\record_structs.py", line 198, in dumpData File "bash\brec\advanced_elements.py", line 940, in dumpData File "bash\brec\basic_elements.py", line 202, in dumpData File "bash\brec\basic_elements.py", line 611, in pack_subrecord_data File "bash\brec\utils_constants.py", line 247, in dump bash.exception.AbstractError: Dumping a dummy fid

Happens because of this PACK:

[image: image] https://user-images.githubusercontent.com/2845240/184477992-17c14791-0eb1-4adb-aae1-0e7892cfa6ab.png

You can see it's lacking the PLDT, so we dump it out. But since it got setDefaulted, it's a dummy FID and errors on dump. I think we could fix this by making the struct a MelOptStruct, but that seems like it's going to turn into another game of whack-a-mole where we get reports of struct X causing dumping errors and we have to just run around the codebase marking things as MelOptStruct. Plus that has another problem: MelOptStruct skips dumping entirely if the values - even if loaded from a previous plugin - match the defaults. Which could break other subrecords when we do it.

Really, the proper solution is the three-tier system: mark elements as required or optional. Required elements always have to be dumped, optional ones only if they were present when loaded. This subrecord, for example, is not required, so we don't have to dump it. Then we can use MelOptStruct for the handful of cases where we really do want to skip dumping entirely if values match the defaults (though I can't think of any places where we actually want to do that...). But then that's way out of scope for now - so what do for the moment?

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

[Infernio]

I changed it in 7fb57e5aac130f3a349f5a234ef78cd343297c87, but feel free to revert that.

[Infernio]

Sorry for merging your branch, was just worried about losing steam on #525 :P

[Utumno]

Truth is I hate merging :P

Pushed some remaining 637 changes - I am still on a break and the setDefault branch will conflict massively (great work on records!) and there are some dark points still for instance in https://github.com/wrye-bash/wrye-bash/commit/7fb57e5aac130f3a349f5a234ef78cd343297c87 we dump IIUC due to the MelUnion.dumpData -> return next(iter(self.element_mapping.values())) - I think the first step towards the finale of #480 should be changing all defaults to None and let dumpData skip the None - for the BP usecase if a "required" subrecord is missing that is a fault of the record loaded, apart from the very few cases we create a record. In the process we should also get rid of MelOptStruct (in 7fb57e5 is essentially used to skip dumping which should be handled with None defaults out of the box)

Then we can use MelOptStruct for the handful of cases where we really do want to skip dumping entirely if values match the defaults (though I can't think of any places where we actually want to do that.

I have some work on that but this involves rebasing the 480 branches so will happen later on :)

[Infernio]

I think there's another regression: MelSorted is sometimes used to sort FormIDs. Sometimes directly (i.e. MelSorted(MelSimpleArray(MelFid(...))) or MelSorted(MelFids(MelFid(...)))) and sometimes via sort_by_attrs to pick a FormID out of a struct.

Previously we'd do that after the FormIDs were converted to short format, but now we do that while they're in long format.

Say we had two FormIDs like this:

('Skyrim.esm', 0x800)
('Dawnguard.esm', 0x900)

Previously this would end up getting converted to short FormIDs and then sorted, meaning they end up in the order of the plugin we're writing's masters:

00000800
01000900

But now we sort the long FormIDs, and those compare alphanumerically, meaning the result is this:

01000900
00000800

Since ('Dawnguard.esm', 0x900) < ('Skyrim.esm', 0x800).

Not sure how to solve this - maybe store the augmented masters of the write context somewhere globally and make FormId.__lt__ use those to look up the master index of its mod_fn and compare that way, falling back to alphanumeric if the masters aren't set (i.e. we're not writing - see e.g. sorted_spells in mergers.py)?

[Utumno]

If this is the only place we sort better modify MelSorted to check if we have a FormIdlist to sort - then we can store the mastertable in the out stream and access it from there?

On Sun, Aug 28, 2022, 11:30 AM Infernio @.***> wrote:

I think there's another regression: MelSorted is sometimes used to sort FormIDs. Sometimes directly (i.e. MelSorted(MelSimpleArray(MelFid(...))) or MelSorted(MelFids(MelFid(...)))) and sometimes via sort_by_attrs to pick a FormID out of a struct.

Previously we'd do that after the FormIDs were converted to short format, but now we do that while they're in long format.

Say we had two FormIDs like this:

('Skyrim.esm', 0x800) ('Dawnguard.esm', 0x900)

Previously this would end up getting converted to short FormIDs and then sorted, meaning they end up in the order of the plugin we're writing's masters:

0000080001000900

But now we sort the long FormIDs, and those compare alphanumerically, meaning the result is this:

0100090000000800

Since ('Dawnguard.esm', 0x900) < ('Skyrim.esm', 0x800).

Not sure how to solve this - maybe store the augmented masters of the write context somewhere globally and make FormId.lt use those to look up the master index of its mod_fn and compare that way, falling back to alphanumeric if the masters aren't set (i.e. we're not writing - see e.g. sorted_spells in mergers.py)?

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

[Infernio]

Not that simple unfortunately, since the FormIDs can come from sort_by_attrs too, e.g. here (from FNV's record definitions):

MelSorted(MelGroups('added_quests',
    MelFid(b'QSTI', 'added_quest'),
    MelGroups('shared_infos',
        MelFid(b'INFC', 'info_connection'),
        MelSInt32(b'INFX', 'info_index'),
    ),
), sort_by_attrs='added_quest'),

added_quest is a FormID, but we retrieve it via attrgetter.

They can even be part of a not-fully-FormID key, e.g. here (from FO3's record definitions):

MelSorted(MelGroups('swappedImpacts',
    MelStruct(b'IMPS', [u'3I'], 'materialType', (FID, 'old'),
              (FID, 'new')),
), sort_by_attrs=('materialType', 'old', 'new')),

old and new are FormIDs, but the primary part of the key is actually materialType, a regular int.

[Utumno]

Thanks for the detailed info - I start thinking that we need to actually drop the comparison special methods - sorting long form IDs does not make sense - sorting short ones should be done by exposing a key function which is essentially the short_mapper

On Sun, Aug 28, 2022, 12:16 PM Infernio @.***> wrote:

Not that simple unfortunately, since the FormIDs can come from sort_by_attrs too, e.g. here (from FNV's record definitions):

MelSorted(MelGroups('added_quests', MelFid(b'QSTI', 'added_quest'), MelGroups('shared_infos', MelFid(b'INFC', 'info_connection'), MelSInt32(b'INFX', 'info_index'), ), ), sort_by_attrs='added_quest'),

added_quest is a FormID, but we retrieve it via attrgetter.

They can even be part of a not-fully-FormID key, e.g. here (from FO3's record definitions):

MelSorted(MelGroups('swappedImpacts', MelStruct(b'IMPS', [u'3I'], 'materialType', (FID, 'old'), (FID, 'new')), ), sort_by_attrs=('materialType', 'old', 'new')),

old and new are FormIDs, but the primary part of the key is actually materialType, a regular int.

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

[Utumno]

Maybe something like:

@@ -155,10 +155,10 @@ def __lt__(self, other):
         try:
-            return self.long_fid < other.long_fid
-        except AttributeError:
-            if not isinstance(self.long_fid, type(other)):
-                if not isinstance(other, (tuple, int)):
-                    raise TypeError(f'Comparing FormId with {type(other)} is '
-                                    f'not supported: {other!r}')
-                raise StateError(f'Comparing {self!r} with {other}')
-        return self.long_fid < other
+            return short_mapper(self) < short_mapper(other)
+        except TypeError:
+            if short_mapper is None:
+                raise StateError(f'Sorting form ids is only permitted in a '
+                                 f'write context')
+            else:
+                ... # incompatible types?
+                raise

[Infernio]

Whew, turns out that's not enough either, because the leveled list patcher and _AMerger actually rely on this alphanumeric sort.

So what I'm doing for now is making FormId.__lt__ do the short-mapping thing in a write context, and otherwise use the alphanumeric sort. This preserves previous behavior. Proper fix is #497, i.e. rewriting _AMerger to not sort and absorbing all mergers into it.

Edit: see 25f7d4cd6baee63a0dea6f82e14ee55fdd8c49b9.