-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: support for behavior mods #659
Feature: support for behavior mods #659
Conversation
Some more info for people interested in testing this, but not wanting to bother with compilation. Please report issues here on github, not to the STR dev team. Or find Ujave on the STR Discord. I have compiled up two versions of this mod and released it on my github; a version with the compilation flag turned on, and a version with it compiled off for testing that it is safe. There's a FOMOD installer provided so anyone can try them. You can find that here. There's not a lot of documentation yet about how to support additional mods, but @MostExcellent or I can help. |
3d9bc9c
to
b89538b
Compare
What's missing from this PR is a general overview of the PR's contents and how you've solved the problem at hand. Currently, I'd have to read the code to do that, which is a bit much given the size of the PR. |
Code/client/ModCompat/magic_enum.hpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a library (in the Libraries folder).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RobbeBryssinck: What's missing from this PR is a general overview of the PR's contents and how you've solved the problem at hand. Currently, I'd have to read the code to do that, which is a bit much given the size of the PR.
I'll do that today. A Theory of Operation type overview, right? It will be a couple of pages or so, in an edited PR description. Most of what's needed is in the code comments, I'll just hoist them and format.
RobbeBryssinck: This [magic_enum.hpp] should be a library (in the Libraries folder).
I'll move it. The original thought was to keep it self-contained, but if it's going to be merged I understand it should be where it belongs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah just a conceptual overview of how the new system works and how it compares to the old system would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested changes are all in. I think this thread can close.
As for concepts, it is additional logic added to the "old" system, which is all still there. So, it's better thought of as an enhancement vs. a replacement.
Well, the Unlimited part is a partial replacement.
007c76e
to
480e299
Compare
56a72bd
to
33ce16b
Compare
Rebased to /master for release. |
33ce16b
to
b5a37c0
Compare
56d3242
to
191a63c
Compare
Updated branch to the latest /dev. It currently compiles with MODDED_BEHAVIOR_COMPATIBILITY undefined. So only Unlimited Vars is compiled, making a standard build server-compatible with a modded animation build. Revert the last commit to build an animation build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing, also we do not use std::map, use the tiltedcore map, as std::map is extremely slow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 2d989d9 addresses most comments, other than clang-format will be an isolated commit to follow. Pushing this now to check CI (formatting shouldn't break CI) and check a few things.
Common Fixes:
- Fixed all gaps in naming conventions; biggest one missing that FunctionsWereCapitalized().
- Replaced all std:: with TiltedPhoques:: functions when possible. Sometimes TP didn't have the function (sorted sets), and sometimes conversions were broken with no workaround.
- Final commit will enable the conditionally compiled code, as that seems to be the consensus.
- Note enabling the code introduces a README-ANIMATION-MODS that will need review. See it in commit 11dc8bc
- There were a few call-by-values that were fixed to call-by-reference to avoid copies.
|
||
uint32_t BehaviorVarsMap::find(const uint64_t acKey, const std::string acName) | ||
{ | ||
auto map = m_map.find(acKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a better variable name would be nice + spacing everywhere is off (empty lines)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a clang format you can execute over the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 2d989d9
A better name than acKey is hard, as the base STR code is using m_key. But, took a crack.
Code/client/ModCompat/BehaviorVar.h
Outdated
std::vector<uint64_t> signatureMatches(const uint64_t acHash, const std::string acSignature) const; | ||
|
||
std::vector<Replacer> behaviorPool; // Pool for loaded behaviours | ||
std::map<uint64_t, std::chrono::steady_clock::time_point> failedBehaviors; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiltedcore map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 2d989d9
Code/client/ModCompat/BehaviorVar.h
Outdated
Replacer* loadReplacerFromDir(const std::filesystem::path acDir); | ||
std::vector<uint64_t> signatureMatches(const uint64_t acHash, const std::string acSignature) const; | ||
|
||
std::vector<Replacer> behaviorPool; // Pool for loaded behaviours |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiltedcore vector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 2d989d9
Code/client/ModCompat/BehaviorVar.h
Outdated
const uint64_t acNewHash, const Replacer& acReplacer, | ||
std::map<const std::string, const uint32_t>& acReverseMap); | ||
|
||
std::vector<std::filesystem::path> loadDirs(const std::filesystem::path& acPATH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding conventions? no camelCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 2d989d9
} | ||
|
||
// Process a set of variables, adding them to the aVariableSet | ||
void processVariableSet(const std::map<const std::string, const uint32_t>& acReverseMap, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs tiltedcore types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 2d989d9
std::vector<std::string> boolVar; | ||
std::string tempString; // Temp string | ||
|
||
std::ifstream fileSig(signatureFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot assume that this succceeds, handle error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 2d989d9
Fixed it. It's a pretty narrow window, though. The path is from a directory iterator just run; so either the file was just deleted in the last millisecond or the directory is read-protected; a lot more would break in the latter case.
// Find any behaviors which match the signature. | ||
// There should be exactly one. | ||
// | ||
std::vector<uint64_t> BehaviorVar::signatureMatches(const uint64_t acHash, const std::string acSignature) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String by copy is a bad idea. maybe a string view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 2d989d9
Code/client/ModCompat/BehaviorVar.h
Outdated
std::string signatureVar; | ||
std::string creatureName; | ||
std::vector<std::string> syncBooleanVar; | ||
std::vector<std::string> syncFloatVar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No std types. use tiltedcore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 2d989d9
I'll run through your review comments. Tried to follow the style guide, but at least missed capitalized funcs/methods. Time to figure out the clang formatter, I guess. Looks like a VS reinstall lost my tabs->4spaces setting. Is that the source of the generic "spacing" comment? And will clang formatter fix it? Otherwise I can do it manually. Looks like I missed converting some contributed code to TiltedPhoques::. But one is deliberate; if I remember right TiltedPhoques::Map and ::Set are hashmaps, and I needed a sorted map / set; is there one? Found TiltedPhoques::SortedMap, but it doesn't look like there is a sorted set. Performance isn't super-critical for these, as they are only used once per creature type per launch (as in, once for all humanoids, once for all dragons...). So even if TP::SortedMap is slower, it won't hurt anything. |
6db7bd0
to
ad2fb18
Compare
ad2fb18
to
5121ad4
Compare
STR is up against the limit, using 62 of a hardwired 64 boolean behavior vars to be synced. This fix removes the limit before it becomes a barrier. Since "unlimited" will bite us, note the base code has some lengths encoded in 16-bit ints that will bite eventually. Trying to sync even a fraction of that many will cause problems, though. Reimplemented the Boolean long long as a (bit) Vector. This made all vars (bools, ints, floats) vectors, so some code simplification of the wire protocol is accessible. The Booleans are just passed "as is," no attempt to detect which ones changed as the space required to send the delta is more than just sending the bits. Overall impact on wire protocol: sending humanoids will be slightly longer, as the 64-bits available is almost full. But that's longer by a byte (or maybe two since I used String to simplify the code). Everything else sent will be shorter and less overhead. There's no avoiding iterating the bits of the Vector<bool>. But the overhead when translating the array of <bool> to an array of bits and back is about the same. There was a bunch of extra overhead if Serialization::WriteBool() were to be used, so optimized that part out. Overall, should be better on the wire, and neutral-to-better on CPU. 2nd commit msg: Reworked creating and applying Behavior variable diffs and serializing them. Simpler, easier to read, should usually save bytes on the wire.
…that it's a Vector; initial load-in animation variables might be empty.
…odded behavior. We want to detect this at runtime (like Edho did) so we can merge the STR team's behavior vars with the modded behavior vars (and not require mods to know what the STR devs picked to sync). This will also give substantial version independence, where this feature should work with multiple versions of STR.
Solve player 1st person woes Lighten intrusion into STR main body (References.cpp) Fixed sorting of AnimationGraphDescriptor behavior variables, this will help when Pandora is more widely adopted (and stops reordering behavior variables, maybe we can get to mod order independence). Added failList of modded behavior lookups that fail, so we don't repeat expensive failed calls. Even some built-in ones can fail, like wisps. Deleted dead code, marked some I'm not sure about for deletion. Update README.md with a decent description of the content and goals of the fork. Cleanups, comments about how it works.
Move SkyrimTogetherReborn\behaviors directory up a level to SkyrimTogetherRebornBehaviors. This gets modders out of the business of changing things in SkyrimTogetherReborn directory, and should make it it easire for mod authors to patch. Created default behavior mod directories for the supported creatures, so modders don't have to find hashes or signatures, they can just drop in behaviorvars they need. Translated Code\encoding\Struct\* files using code generation PowerShell script. Eventually will run automatically. Eventually may get STR Dev permission to eliminate it. Now when finding modded behavior with game base behaviors, translates the "old" numeric BehaviorVar values back to strings, then looks up the string to get the new numeric value. This means mod developers no longer need to know which variables STR Devs require. Better warning messages when lookups fail to aid hunting down the issue.
…game (lower case "speed"). If we get a miss on Speed try again with "speed". If this is the only one, this is a safer fix than going to case-insensitive search, which could have its own issues. Raised the log level to Error for these sorts of things.
Support remembering the modded humanoid/Master hash so exiting a beast modes can return to that (vs. a force-plug of the original unmodded behavior).
Greatest changes are vastly improved error messages to warn when signatures are not unique (match multiple creatures). Those messages made possible improving many signatures. They also showed that 8 creatures simply are not unique; the only OrigVars they define are shared. Sometimes rearranged, but they all have the same 92 variables. A mod to one of these creatures will have to define a new unique signature, even if it means introducing a variable.
Incorporating MostExcellent's PR. We've observed several instances of the stock game having typos in variable names, like improper capitalization and the like. Tolerate case-sensitive misspellings. if a vanilla behavior variable isn't found, capitalize the first letter, the most common misspelling. Then try all lower case, then case insensitive. Things would be a lot easier if people paid attention to case-sensitive variable names.
… compile, and better mark the dead code for deletion.
…IONS-MODS.md with updated information.
…ehavior is modified (it always is with Nemesis installed). And fixed a case where humanoids exiting beast mode would similarly fail if Nemesis wasn't installed.
…rings must be compared case-independent to match.
…ev-selected variables are now merged with what the mods need, so it is no longer necessary to request inclusion. And it's probably confusing as they shouldn't be there (and devs could change them).
…COMPATIBILITY. Should be an xmake option, but that doesn't seem to work properly.
… now. clang-format will get an isolated commit. Replaces lingering std:: types with TiltedPhoques:: types wherever feasible; there are a couple of cases where TP is missing function or breaks type conversion. Applies style guide naming conventions where there were misses. Fixes missing failure check on file opens. The paths must exist (they are from a directory iterator), but read permission could be missing. Cleaned up readme.
Also rebase to lateest /dev
…t always compiled now.
New to remember the modified MasterBehavior hash, since it is used to return from Beast mode. Previous implementation stored in ActorExtension, but would only get set on the first modified humanoid which might not be the PlayerCharacter. Similar issue for dragons, remember the modified hash so Actor::IsDragon() test will work for all modded dragons, not just the first one.
e751157
to
479ca9f
Compare
Behavior mods such as those generated by Nemesis or Pandora.
Important note: this PR supports conditional compilation, which is currently set to off in the last commit. This was done to enable the code to merge without risking the imminent release. If the conditional compile is switched on, the support will be enabled in the clients and those clients should be able to use the same servers as clients without the support.
Goals
Prerequisites
Testing
Four combinations were tested to ensure the PR is safe:
Theory of Operation (how it works)
It's a big PR in line count, but much of that is machine-translated code. The major topics to cover are:
Directory Layout / Changes
Source Directories
Installation Directories
Theory of Operations
In a sentence, the mod works by detecting AnimationGraphDescriptor hashes that don't exist in base STR, seeing if the Actor's animation variables match a signature in the SkyrimTogetherRebornBehaviors directory, and if so, merge the STR variables with the ...Behaviors directory variables (if any, it might just be the hash was changed), and generates a new AnimationGraphDescriptor with the new hash.
The interception requires a few lines of code in References.cpp to intercept where animation variables are prepared for serialization and deserialization. They call out to BehaviorVar.cpp:BehaviorVar::Patch() when the AnimationGraphDescriptor lookup-by-hash fails.
::Patch can fail and return NULL again. This is normal; it happens when STR doesn't need to sync behaviors, syncing position and velocity is enough, so there is no AnimationGraphDescriptor at all for that creature. Wisps are an example.
If a hash lookup fails, it is added to a failed list and ignored for 10m. This because searching for a signature is semi-expensive, but more because it generates a ton of logs that aren't useful which is the real cost. 10m is long enough for most encounters to end, but short enough to try again if dynamic behavior modification is ever properly supported.
Intricacies of Merging Behavior Var Lists
Nemesis doesn't just add more behaviors to the end of the list; it rearranges the list. That's unfortunate, and it is why humans skate if just Nemesis is installed without adding any more variables to be synced; The rearranged list has a different hash.
To accomplish a merge, these operations occur:
From that point forward, the new hash will be successfully found by the STR base-game logic.
There are a few details that added some complexity. In particular, there are misspellings of variable names even in vanilla Skyrim, usually incorrect case like speed when the correct variable is Speed; that happens with a particular Winterhold guard, for example. Having no way to know if Skyrim ever uses any variables only distinguished by capitalization, the code tries a few common variants if a lookup fails and generally will correct the problem.
Decoupling STR dev team and Modders
All this logic to match and merge is to ensure the STR dev team and modders can make changes independently without interfering with each other.
Support for Syncing Additional Behavior Variables
All of the above support will ensure animation sync if Nemesis/Pandora are used, and if no new variables need to be synced will even be compatible with the same servers used by clients without this animation support.
But to also support mods that do need to sync additional variables, such as the popular True Directional Movement, this mod needs support for syncing more variables. In particular more Boolean variables, STR base uses 62 out of a limit of 64. The limit is baked into protocol between the clients and server, so lifting the limit creates a compatibility problem.
This PR includes a merge of branch feat-unlimited-behavior-vars, which always compiles once this PR is merged to ensure versions compiled with behavior mod support will still be able to use the public servers of the same version.
Rather than simply turning the boolean long long in the code into an array, this code changes the definition AnimationVariables::Booleans from a uint64_t to a TiltedPhoques::Vector< bool >. This compiles into an array of bits with the current tech stack (this isn't guaranteed by the C++ standard, but is the norm).
This design choice was made because: