search

powerof3 / BaseObjectSwapper

MIT License
13 stars 5 forks source link

multiple base forms with random assignment #10

Closed NickStefan closed 4 months ago

NickStefan commented 4 months ago

What

I'd like to modify BaseObjectSwapper to accept multiple base form IDs. When the library encounters multiple base form IDs, the library would then randomly assign each base form to one of the multiple swap ids.

[Forms]
DoomstoneWarrior,DoomstoneMage,DoomstoneThief|DoomstoneWarrior,DoomstoneMage,DoomstoneThief

For example, this INI could randomly assign DoomstoneWarrior to DoomstoneMage, and then DoomstoneMage would be randomly assigned to one of [DoomstoneWarrior, DoomstoneThief], since DoomstoneMage has already been assigned.

Why

While it's true that the existing library supports enumerating each swap by hand, you'd have write out each permutation by hand if you wanted to see a different permutation on your next playthrough.

[Forms]
; Character Bob
; DoomstoneWarrior|DoomstoneMage
; DoomstoneMage|DoomstoneThief
; DoomstoneThief|DoomstoneWarrior

; Character Tim
; ...changing it up...
DoomstoneWarrior|DoomstoneThief
DoomstoneMage|DoomstoneWarrior
DoomstoneThief|DoomstoneMage

Basically, I think there's room for a swap type that randomizes swaps while also guaranteeing each swap only occurs once.

Details

Player Provided Seed String: I originally wanted to seed the RNG with the current save game's PlayerID, but that doesn't work since BaseObjectSwapper runs before loading specific characters. Then I tried using the randomHash RNG, but that doesn't work well since activators seem to remember whatever their script properties were on the very first game load. Finally, refHashing makes the swapping static across multiple characters, which also didn't feel right. So I settled on accepting a user provided RNG seed like so:

[Forms]
DoomstoneWarrior,DoomstoneMage,DoomstoneThief|DoomstoneWarrior,DoomstoneMage,DoomstoneThief|NONE|NONE|randomseedstring

This means you can get a completely new set of swaps just by changing the seed string in one place.

OrderedSet: As I said earlier, activators go nuts if you swap them more than once on a play through. So, for that reason, I wanted the swaps to be deterministic given a specific, player-provided seed string. That means order matters. Therefore, I added an OrderedSet. If that's a deal breaker, I had a version that seemed to work using the unordered set library as well (I guess since the hashing is consistent?). It just felt kind of wrong to rely on read order from an unordered set library.

Testing

Thank you for providing a working CI pipeline. I was able to build and test various test cases on 1.6.640. I originally had more logs for testing, but removed most of them.

While I am a developer, I'm not as experienced with C++, so please feel free to optimize or change the code as you see fit. I wont be offended :).

[Forms]

; seed is NONE
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|NONE|NONE

; seed is empty
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|NONE|

; swap set isnt a set, just one formid
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior|NONE|NONE|randomized

; swap set is shorter than base set
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage|NONE|NONE|randomized

; duplicates, swap set twice as long
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent,DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|NONE|randomized

; CHARACTER 1
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|NONE|randomized

; CHARACTER 2
DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|NONE|random

;[References]
;0x000E7BDD~Skyrim.esm,0x000E7BD6~Skyrim.esm,0x000E7BDC~Skyrim.esm,0x000E7BDC~Skyrim.esm,0x000DC84A~Skyrim.esm,0x000E0DC2~Skyrim.esm,0x000E0DBE~Skyrim.esm,0x000E0E1F~Skyrim.esm,0x000E0F4A~Skyrim.esm,0x000D1E9F~Skyrim.esm,0x000E0F22~Skyrim.esm,0x000D9630~Skyrim.esm,0x000E0F6F~Skyrim.esm,0x000E0F55~Skyrim.esm|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|NONE|randomized
powerof3 commented 4 months ago

Thanks for the PR.

RE: the seed string, it might be better to make it part of CHANCE rather than an entirely new section. Something like ChanceXXXX(100)? Also any reason you're using a string and not an integer directly?

NickStefan commented 4 months ago

Thanks for looking at this.

I'll try and fold the seed into the CHANCE section. Given that, I agree that providing a seed integer makes more sense since the chance section deals with numbers. There wasn't a great reason for making a string.

NickStefan commented 4 months ago

I've updated the code so that the seed is now an optional argument when setting a chanceR value, e.g. chanceR(80, 1234).

Passing the seed to chanceR should also work for swap types that predate this PR (Barrel01|Barrel02|NONE|ChanceR(80,12345)). This provides an alternative to using the refHash; it can be randomly changed by changing the seed, but it's also going to be stable across save games and game loads.

For the feature I originally proposed--multiple base forms with random assignment--it allows doing something like this:

DoomstoneWarrior,DoomstoneMage,DoomstoneThief|DoomstoneWarrior,DoomstoneMage,DoomstoneThief|NONE|chanceR(100,350)

This would randomly swap these, using 350 as the RNG seed when assigning pairs.

LMK what you think and thanks for the earlier feedback.

Some manual testing that I did:

[Forms]

; seed is NONE, defaults to RefHash randomization
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|NONE

; swap set isnt a set, just one formid, logs handled error
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior|NONE|NONE

; swap set is shorter than base set, logs handled error
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage|NONE|NONE

; duplicates, swap set twice as long, should work same as first example
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent,DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|NONE

; CHARACTER 1, specific seed
DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|chanceR(100,350)

; CHARACTER 2, specific seed
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|chanceR(100,850)

; Random every launch of skyrim executable
;DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|chanceR(100)

;[References]
;0x000E7BDD~Skyrim.esm,0x000E7BD6~Skyrim.esm,0x000E7BDC~Skyrim.esm,0x000E7BDC~Skyrim.esm,0x000DC84A~Skyrim.esm,0x000E0DC2~Skyrim.esm,0x000E0DBE~Skyrim.esm,0x000E0E1F~Skyrim.esm,0x000E0F4A~Skyrim.esm,0x000D1E9F~Skyrim.esm,0x000E0F22~Skyrim.esm,0x000D9630~Skyrim.esm,0x000E0F6F~Skyrim.esm,0x000E0F55~Skyrim.esm|DoomstoneWarrior,DoomstoneMage,DoomstoneThief,DoomstoneLady,DoomstoneApprentice,DoomstoneLover,DoomstoneSteed,DoomstoneAtronach,DoomstoneShadow,DoomstoneLord,DoomstoneRitual,DoomstoneTower,DoomstoneSerpent|NONE|chanceR(100,235663)
powerof3 commented 4 months ago

Looks good to me.

powerof3 commented 4 months ago

Feature has been released. Can I have your Nexus username?

NickStefan commented 4 months ago

Thank you.

Looking at the Base Object Swapper mod page, I believe that the changelog/docs are slightly incorrect with what was merged.

I think the example given will actually log a handled error "SWAP formID set not found".

When we find a FormIDSet for the first ini argument, we expect another FormIDSet for the second argument. So the second argument actually gets turned into an empty set right now if it's only one FormID. This is arguably a bug that I can fix.

origBaseID,origBase2ID,origBase3ID|swapBaseID|propertyOverrides|chance

https://github.com/powerof3/BaseObjectSwapper/blob/master/src/SwapData.cpp#L96

Given an empty set, the code then steps over the if statement and on to log an error message.

            if (auto swapFormIDs = util::GetFormIDOrderedSet(formPair[1]); !swapFormIDs.empty()) {
                if (baseFormIDs.size() > swapFormIDs.size()) {
                    logger::error("\t\t\t\tfail : [{}] (SWAP formID set must be equal or larger than BASE formID set)", a_str);
                    return;
                }

                             // ...
                             // ...
            } else {
                logger::error("\t\t\t\tfail : [{}] (SWAP formID set not found)", a_str);
            }

So right now it works like: origBaseID,origBase2ID,origBase3ID|swapBaseID|propertyOverrides|chance -> error

origBaseID,origBase2ID,origBase3ID|swapBaseID,swapBaseID2,swapBaseID3|propertyOverrides|chance -> randomly assigns each base to one swap.

I think it would be pretty clean to handle "normal" swaps when the swapBaseID is just one formID. So then we'd have:

origBaseID,origBase2ID,origBase3ID|swapBaseID|propertyOverrides|chance -> swaps all of the origBaseIDs to swapBaseID

origBaseID,origBase2ID,origBase3ID|swapBaseID,swapBaseID2,swapBaseID3|propertyOverrides|chance -> randomly assigns each base to one swap