-
Notifications
You must be signed in to change notification settings - Fork 21
More enviromental awareness #104
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
Conversation
- supports Frostfall now if available, people will be aware of dangerous weather, coldness and wetness - added information to learn things like if someone is sitting, sleeping, mounted on a horse, swimming, dressed warmly, has ever been intimidated by the player, finds the player potentially intimidating, what 'career' someone has (if its not secret like thief or vampire)
…statements about the actor's temprature, got more specific language
added support for moon cycle
| EndIf | ||
|
|
||
| ; has player ever bribed this NPC? | ||
| bool bIsBribed = false |
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 will probably run too infrequently for more dynamic things like bribing / intimidating. If the player does do those, it may be minutes/hours until the LLM is aware that they did I think.
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.
I think for most Intimidation moments, they may have to play out as the adventure writer wrote them, and have the LLM pick up on them there.
Maybe intimidation and a couple of other values can get a 1 minute update cycle independent of others?
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. I think that makes sense.
| return name | ||
| EndFunction | ||
|
|
||
| string[] function lengthenArray(string name, string[] nameList) |
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.
The cache + mechanism by which we're extending it here is rather unperformant. This is an unbounded cache, and we're having to copy the entire list every time a new npc is added.
Also (And I don't actually know the answer to this one off hand) - How does thread safety work with this in Skyrim? If you enter a zone, and sapience adds 10 npc's, and all of them begin executing SetContext, it seems that you may have some thread safety issues on working on the array like this.
We may want to consider an alternative data structure and/or some sort of retention mechanism.
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.
I've read a FormList is more ideal, I'll use that
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 could also consider using a jcontainers object, since it's already a core dependency. I'm using those in a few other places in the mod, and they seem rather performant.
| // we already did the player -- this can come up if the player forgets to set their name right in CHiM | ||
| $bValidToInclude = $actor!="" && $player_name != $actor && !in_array($actor, $dedupe); | ||
| if($bValidToInclude) { | ||
| $new_text = $utilities->GetActorValue($actor, "EnviromentalAwarenessMoreStableData") . $utilities->GetActorValue($actor, "EnvironmentalAwarenessDynamicData"); |
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.
One general comment - This isn't a blocker per se, but let's talk about persisting strings from Papyrus rather than persisting values, and then building the strings from php.
Earlier on in the mod's development (when Mantella support was a priority), I took a similar string-building approach in Papyrus. This comes with a few downsides:
- Papyrus performance sucks, and it's bad at string operations. Doing the string operations in php reduces game load.
- If we want to reference things like bIsIntimidated to combine with other integrations for better context awareness, it's a bit more difficult to do. For example, we could perhaps add some sort of awareness to scenes if the player had intimidated the npc.
It does have the advantage of making less calls to the server / skse dll, however.
Could you explain your rationale for this design decision?
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.
Which design decision? Doing strings in papyrus? or something regarding lines 26-29?
Strings - I don't really know much about Papyrus and its qualities. I wouldn't want to write and read 30 different values from the db on each cycle for each actor, but it could be serialized into one field. Papyrus would still end up taking the same number of if/then branches though. As far as where the code was, I thought it was just organized having the PHP mostly read from the DB and output it.
As far as checking NPC clothes that aren't Followers, I assumed they mostly never change - was that wrong?
IsIntimidated and perhaps some others can be put of a more reliable update cycle. Is there anything like hooks?
"akActor.OnIntimidated( alert the LLM) "
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, valid. We're already persisting quite a few different values per cycle for actors in all of the spread out SetContext's. I was thinking about adding a new function to the CHIM DLL to allow us to pass in a jcontainers object, and have it serialize / write the jcontainers object to the server in one shot to improve performance (And, implementing a custom routine on the server to handle this). One other thing to consider on your current approach, is that if you / anyone does want to tweak the llm prompt that's sent, it's rather difficult for them to do so, as editing the papyrus is a lot harder for people than editing the php. Again, not a blocker, just something to consider. I'm fine with us committing a string based approach for the time being.
There are indeed hooks that we can leverage for this sort of thing - They're called Events in Skyrim. One of the things I've been meaning to do more generally is move more stuff out of SetContext, and to an event driven system to improve performance.
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.
Here's a list of most of skyrim's supported events: https://ck.uesp.net/wiki/Category:Events
There are others that are added by some of our dependencies as well.
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.
The changes we've discussed are in, it doesn't talk clothes other than to say if the look poor or rich (if those qualities present), weather was moved to weather section, and uses JContainer instead of native-to-papyrus array
| if(bNotInList) | ||
| wasInList = an + " was NOT in list" | ||
| endIf | ||
| MinaiUtil.Info("Environmental Awareness SetContext 002 :: " + wasInList) |
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 seems like it should be a debug instead, as it will be pretty spammy, right?
| function GetEnvironmentalContext($localActors, $targetActor) { | ||
| $utilities = new Utilities(); | ||
|
|
||
| // it doesn't matter if Frostfall exists, this PHP loads sentances verbatim from the db |
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.
Ideally we should be constructing these server-side rather than loading them from the db. That way, we can access that information from other scripts as well, and expose these variables to other modders that want to translate, etc.
adding context about people in the environment, and pulling data from Frostfall when it exists
Frostfall
Environmental Awareness