Add a way to set Saliency Strategy from godot without having to write C# #85
Comments
|
This could also be a field on the DialogueRunner itself, but that becomes iffy if people decide to set a custom strategy, while this extra node is totally optional. I suppose the enum could include a "Custom" value which doesn't set anything if the on-runner approach seems better. |
|
I tried this out locally, but when adding this to the space sample scene and hooking up the inspector fields, it crashes the game when talking to the Sally character, because Program in the variable storage somehow becomes null. I have no idea why though . The existing one liner added to SpaceSample.cs doesn't cause the same crash. This is my version of your script that seems to be causing the Program to become null somehow as a side effect using Godot;
using System;
using Yarn.Saliency;
namespace YarnSpinnerGodot;
/// <summary>
/// Lets you set the saliency strategy of the chosen DialogueRunner.
/// Saliency Strategy determines how group nodes are chosen.
///
/// Note: Does not support custom saliency strategies. For that use case you would want your own script
/// that implements and sets your custom strategy.
/// </summary>
[GlobalClass]
public partial class SaliencySetter : Node
{
public enum SaliencyType
{
First,
Best,
BestLeastRecentlyViewed,
RandomBestLeastRecentlyViewed
}
[Export] public DialogueRunner dialogueRunner;
[Export] public VariableStorageBehaviour variableStorageBehaviour;
[Export] public SaliencyType SaliencyStrategy = SaliencyType.First;
public override void _Ready()
{
if (!IsInstanceValid(dialogueRunner))
{
GD.PushError($"{nameof(dialogueRunner)} is not set on {nameof(SaliencySetter)}! You must set it to use this script");
return;
}
if (!IsInstanceValid(variableStorageBehaviour))
{
GD.PushError($"{nameof(variableStorageBehaviour)} is not set on {nameof(SaliencySetter)}! You must set it to use this script");
return;
}
IContentSaliencyStrategy chosenStrategy = SaliencyStrategy switch
{
SaliencyType.First => new FirstSaliencyStrategy(),
SaliencyType.Best => new BestSaliencyStrategy(),
SaliencyType.BestLeastRecentlyViewed => new BestLeastRecentlyViewedSalienceStrategy(
variableStorageBehaviour),
SaliencyType.RandomBestLeastRecentlyViewed => new RandomBestLeastRecentlyViewedSalienceStrategy(
variableStorageBehaviour),
_ => throw new Exception("Unknown Saliency Strategy")
};
dialogueRunner.Dialogue.ContentSaliencyStrategy = chosenStrategy;
}
} |
|
Hmm weird. It looks fine, and it worked just fine dropped into my project. I tried getting the samples working but failed; I'll try again tomorrow to see if I can repro. Possibly some change between your latest and what I pulled down causing the issue? |
|
Or maybe a race condition if multiple things are trying to mess with the dialogue runner on ready 🤔 |
|
Okay had no issue using this code when pulling down from the PR. Going to try latest. |
|
Are there docs somewhere for getting latest to build? It complains about a lot of missing dependencies, only some of which I'm finding on nuget. |
|
I hit the error when running the Space sample talking to the Sally character on the left and choosing the first dialogue option. What do you mean by latest versus the PR? The PR is currently the latest version of v3. All dependencies in the v3 branch are available on nuget (see CI builds) |
|
If you mean the develop branch that's the 0.2 version which doesn't have the saliency strategy concept |
|
Ah, I'm just a little confused by github. I don't usually use it for much more than my own stuff. (Azure for work heh.) So if what I download here contains all the latest commits, then everything works fine for me. I'm able to start the dialogue, talk to Sally, select the first dialog options, get a response ("Same old nebula" etc.) How do you have your scene set up for testing this? Could you send me a version of the scene that has the error so I can dig into what the difference might be? This is my setup:
Also, what strategy did you select? Could be some issue with the strategy chosen, I'll try the others when I get back to my PC. |
|
Okay, all four saliency strategies worked fine, I was able to progress through the Sally dialog without issue. |
|
I'm not sure why the difference. My setup seems the same as yours. Are you using a different Godot version? I have 4.3 stable
|
|
Mysterious! I'm on Do you have any other local changes, either to code or to the script? Does this happen with a fresh download of the PR with nothing added but the SaliencySetter code? |
|
The only other change I have is removing setting the saliency from SpaceSample since that would be redundant
|
|
Aww heck yeah that was it, I get the error now too! I have some time now so am gonna ⛏️ |
|
Thanks |
|
Hmm okay some data: SpaceSample.cs, when passing the variable storage to the saliency object, uses So this fixes the problem: Inspecting things, it doesn't look like the two objects are actually the same? One is a Godot Node, the other is just a |
|
Before anything happens, the exported |
|
Huh that is odd and I don't know why the difference either but maybe the fix is just to use the same way of setting the variable storage on the saliency strategy |
|
Okay it's getting weirder. This ALSO fixes it: Just adding the GD.Print. wtf. race condition??? |
|
I do think using the dialogue runner's variable storage feels really safe, bc there doesn't seem to be any reason to set different ones except that you like errors. Does accessing the dialogue runner's variable storage have some kind of side effect to "set up" the storage? |
|
Oh it shore does |
|
That should only make a difference if there was no variable storage set on the dialogue runner though . I'm not sure what is going on |
|
You're right, that code is skipped. Jumped the gun. Here's a weird error:
Idk what to make of that. Can the Node part of an object be disposed while the C# object remains? |
|
Using a basic I did some checks and it doesn't seem like a race condition despite having all the stink of one, so some kind of weird combination of order of operation and how Godot and C# inheritance interact? 🙃 I mean using dialogueRunner.VariableStorage seems safer and better and works, so maybe just do that... but I'll keep digging for a bit, it's frustrating not knowing why. |
|
Okay, wait, maybe my print statements are throwing an error and stopping execution of the broken variableStorage usage. Hmm hmm. I forget that Godot keeps running through errors. |
|
Sorry you're getting like a live feed of my debugging this >..< |
|
Okay, ignore all the GD.Print "fixes" -- those are the ramblings of a confused dummy. They throw errors, which breaks in the function but has the effect of just not setting saliency. But at least that makes things somewhat less mysterious. |
|
Okay. I think there's something borked with DialogueRunner.VariableStorage. When DialogueRunner is instantiated, it assumes the presence of a VariableStorage: But that GD.Print will fire, because actually VariableStorage is not valid at the time this is called. But the constructor looks like this: But I don't think ?? is valid here, because is IS an object, some kind of Godot object, that has no internal value yet but is NOT So I think VariableStorage gets set to this weird empty-but-not-null object, which is not the same as the actual node we pass in successfully in SaliencySetter. More evidence, in Player.cs: That inappropriate GD.Print will fire every time. We don't actually have correct VariableStorage here, even though it looks like we do. Final evidence, this single line of code in SaliencySetter.cs fixes the issues: Full ready function I'm testing: |
|
So my theory is that the way DialogueRunner.cs tries to set its VariableStorage using the export is flawed and creates this strange zombie object that is a placeholder(?) for the export, but is not a reference to the exported value that will eventually be there. OR SOMETHING. But I do think it's being set wrong, maybe before the node is ready to be looked at. |
|
I have reverted all my test changes and JUST added ...and it works. |
|
Thank you for your help trying to investigate this. May I ask that you please try to wait before posting and consolidate your findings some so that I'm notified less often? No hard feelings ! I actually think this is some kind of race condition with multiple nodes running code in _Ready at the same time. Without the Saliency setter script , the variables storage is set properly. The checks you are talking about where variable storage is not null do not fire in normal usage when you run dialogue - if you have set a variable storage in the DialogueRunner's inspector, it works as intended. I think what is happening is the Saliency setter is trying to access the variable storage too early. I wonder if the original code I posted would work if the last part, the switch statement and |
|
Yes, heard chef. I did realize I was spamming and tried to step back, thus the longer post with better info :) I think I have the fix. It is some kind of race condition, and it's specifically an issue with So to resolve the issue even when not local, I added this code to SaliencySetter.cs: Here is the complete class: In other findings, even without a SaliencySetter this error from Player.cs will print: I have no idea what this means. Maybe it means nothing. I just find it strange. |
|
Thank you! I'll use your updates and credit you in the next release and also take a look into the Player.cs situation |
|
Yeah sorry I'm such a sloppy hot mess on the way but I get there eventually. |
|
Thanks again, now available in #75 |
For folks who don't want to deal with C#, I suggest adding a simple way to set the Saliency Strategy of their DialogueRunner instance, via a node script, something like:
The text was updated successfully, but these errors were encountered: