Mutagen-Modding / Mutagen

A .NET library for analyzing, creating, and manipulating Bethesda mods
GNU General Public License v3.0
121 stars 32 forks source link

ActorValueInformation -> ActorValue mapping #447

Closed Noggog closed 1 year ago

Noggog commented 1 year ago

Add some utility mapping between the two concepts so you can get the enum if you have the record

Noggog commented 1 year ago

Put some outline work in actor-value-mapper branch

MarkKoz commented 1 year ago

I'm interested in working on this.

What's the suggested strategy for testing this without essentially reimplementing the mapper in the test?

Noggog commented 1 year ago

I dont know that testing is too useful here, as it's basically just going to try to catch typos. But your test is only good assuming it doesn't have typos itself. The chances you getting typos in the actual code is the same as you getting typos in the test, so it's not really adding anything.

If the mapping is typed in wrong, it's typed in wrong, I'd say

MarkKoz commented 1 year ago

I wrote a quick script to match them up based on editor ID, since the names are easier to verify by eye than form IDs. I used a script in SSEEdit to export all the AVIF records from Skyrim.esm/SkyrimSE.exe and then piped that CSV into my script.

Results are that the current ActorValue enum is missing the following:

000005D5 AVPerceptionCondition
000005D6 AVEnduranceCondition
000005D7 AVLeftAttackCondition
000005D8 AVRightAttackCondition
000005D9 AVLeftMobilityCondition
000005DA AVRightMobilityCondition
000005DB AVBrainCondition
000005E2 AVIgnoreCrippledLimbs
00000649 AVDEPRECATED05

Do we want to add all these missing values or were some intentionally omitted? These are all obsolete according to the wiki, but we do already have some values in the enum that are obsolete.

Also, ActorValue.MarksmanSKillAdvance has a typo ("K" in "SKill" should be lowercase), but I suppose at this point it's too late to change as it'd be a breaking change.

Noggog commented 1 year ago

Do we want to add all these missing values or were some intentionally omitted?

The ActorValue enum was originally written from these xEdit defintions. https://github.com/TES5Edit/TES5Edit/blob/dev-4.1.5/Core/wbDefinitionsTES5.pas#L6103

It is an enum that exists within the Bethesda system, separate from the ActorValueInformation records. And I dont think there's a guarantee that all ActorValueInformation must have a value in the enum? For example, we could define a new ActorValueInformation in a mod we make, and it for sure wouldn't be on the enum.

That being said, that's all my gut thoughts ^. There might be some research to do a bit and maybe there is some missing entry on the enum that aligns with a few of those. But that'd be something someone would have to decide to sink time into.

Also, ActorValue.MarksmanSKillAdvance has a typo ("K" in "SKill" should be lowercase), but I suppose at this point it's too late to change as it'd be a breaking change.

There's a reason Mutagen is still v0.X, haha. Fix the typo! I don't mind backwards compatibility breaks, generally. Even if it was 1.x, especially in the record definitions. These are going to happen all the time in a library like Mutagen, and the systems are generally designed to handle small breaks like this (Synthesis/Spriggit versioning systems, etc).