EDCD / EDDI

Companion application for Elite Dangerous
Other
443 stars 81 forks source link

Actuality of the state variables (e.g. eddi_context) #2647

Closed nepomuk16321 closed 1 month ago

nepomuk16321 commented 1 month ago

What's Wrong (please be as specific as possible)

Hi T'Kael, I have a problem with the actuality of the state variables (e.g. eddi_context). Often the variables are not up to date and EDDI (for me/us GerDA :-) ) tells me old familiar things. One example is the script “Star scanned” with the follow-up script “Star report”. The variable {SetState('eddi_context_star_star', event.bodyname)}, which is set in the “Star scanned” script, is used in the “Star report”. In my case, your test star “Cota” was often announced as the current star (the status variables such as status.fuel_percent are also affected). Do you know the problem? Can you do something about it? Are there perhaps too many variables or is the processing not fast enough? I also thought at first that the journal might not be fast enough, but that's not the case. Do you have any ideas?

Translated with DeepL.com (free version)

Expected

The results of the query or the readout of the state variables should represent the current status as far as possible.

Observed

... see above in the text

Steps to reproduce

... see above in the text

Configuration

My Investigation

Investigation Notes

Test of the jounal entries, functionality of the API Ok, I haven't done a test with the original character yet (I'll do it soon)

EDDI Logs

No errors in the eddi.log (normal logging)

nepomuk16321 commented 1 month ago

I did another test. I read and announced the state variable {SetState('eddi_context_star_star', event.bodyname)} directly in the “Star scanned” script with {GetState('eddi_context_star_star')}. The name of the star was immediately announced correctly. The follow-up script “Star report” (I call these scripts function scripts because they are called with {F(“Script name”)}) was again unable to display the variable correctly (this time not at all). Could it be that the error is there, difference between the event scripts and the function scripts? After a manual call (“Test script” button) of the “Star report” script, the star was then announced correctly.

Darkcyde13 commented 1 month ago

I've had a similar experience, but on investigation, I believe this is the intended behaviour of the new Cottle speech responder. This is from the alpha 1 change log:

Custom Functions A variable set with SetState can no longer be retrieved using state from the same script / context where it was set. GetState has been added to allow retrieval of state variables in the same script / context where it was set.

I've had to change a few instances in my personality because state. no longer works the way it used to. Some of those are my own scripts, and some that are default scripts are because I've made changes in them, but if I remember correctly, there are still some that need the default updating to work properly again.

What I've found is that you can set a State() variable in a script, but not use it in the same script or one that is called from that script. If you try, you will get the previous value of that variable. So, for example:

1st run: SetState('variable', 1).  state.variable = void  GetState('variable') = 1
2nd run: SetState('variable', 2).  state.variable = 1  GetState('variable') = 2
3rd run: SetState('variable', 3).  state.variable = 2  GetState('variable') = 3
etc.

The default Star report sets reportBody from the state.eddi_context_star_star variable, and as the script is called from Star scanned where that variable is set, the state. variable probably contains the previous data, and I think you will need to use GetState() instead to read the current data. I've not noticed if my version is having this problem, so I'm going to go check it out now. Looking at my code, I think it will need updating. EDIT: I've had to update the Body report and Body report summary scripts for this exact reason, so I'd say Star report will need to be updated too.

nepomuk16321 commented 1 month ago

Hi @Darkcyde13 , yes, now as you describe it, that could be the error or the problem. But I think I had tested that ? ... I'll have another look at it. Thanks for the direct hint. (I'm desperate at the moment because I'm trying to find the cause. The second time the script is called, the values are correct again tzzz ... )

EDIT: So, after a few jumps, it is now clear to me that my problem was reading the state variables in subsequent scripts. Changing the reading of the variables to ‘GetState(’ xxx ‘)’ has helped. Thanks again ! (Now I have to check the other scripts ...)

Darkcyde13 commented 1 month ago

I've finally had time to test this properly myself, and I was correct in my assumption. The Star report does indeed fail due to the use of state. instead of GetState.

I added a line to Star report to speak the context variables used to get reportBody, loaded my game, jumped into a system I've not been to and needed scanning, and after the main star had been scanned, the report did not voice either of the context variables, I manually ran it again with the test button, and THEN the star name worked, but the system name did not.

Looking into it, I found that the use of eddi_context_star_system also fails due to the event variable that it is set to no longer existing in the Star scanned event.

{SetState('eddi_context_star_system', event.systemname)} in the default Star scanned script does not work as event.systemname doesn't seem to be a valid event variable. Maybe it used to be once, but it's not listed in the 'variables' anymore, and in-game does not get set to anything. Or maybe there is some other bug in EDDI where it should be available, but isn't?

I'm beginning to think I need to go over all scripts that use a state. variable, to check those that are called from other scripts where that variable is being set, and then set them all to use GetState instead.

nepomuk16321 commented 1 month ago

... unfortunately, that is also my fear ... I have made the same observation that you describe. I changed the variable in the ‘Star report’ and ‘Star scanned’ script event.systemname to system.systemname. This then worked. Actually, the name of the system should already be set after the jump to the system, i.e. with the ‘Jumped’ script. There are the event variables event.name and event.star, but here the state variables are called differently, namely {SetState(‘eddi_context_system_name’, system.name)}. I think there is a lot to do.

Darkcyde13 commented 1 month ago

Yes, I agree. In my versions of Star report and Star habitable zone, I have changed the reportBody to use eddi_context_system_name. As you say, that is set in Jumped, so it makes sense to use it.

Tkael commented 1 month ago

Hi there. I've been out of town for a few days and am just getting back. Looking over this, I can confirm that event.systemname in the Star scanned event ought to be a valid property and currently is not a valid property. Consequently the value of that property is always null. I'll be fixing that for the next release. This is separate from any issue you might be encountering with state variables (please let me know if you encounter anything else that is behaving unexpectedly?).

As Darkcyde noted above, SetState no longer affects the immediate script context and GetState has been added if you need to retrieve a state value set in the same script. GetState is not necessarily needed in the Star report script however because it is invoked via the F function which creates a new context which includes changes which were already made to state variables via SetState in the parent script.

nepomuk16321 commented 1 month ago

Hi @Tkael , no problem at all. Summer is so beautiful, always use it when you can! My problem: Now I'm a bit confused. This does not mean that in subsequent scripts that evaluate a SetState() variable, this evaluation must be changed to GetState(). So the evaluation can remain in state.xxx ? The problem with the “Star report” script is solved, in my opinion.

Darkcyde13 commented 1 month ago

GetState is not necessarily needed in the Star report script however because it is invoked via the F function which creates a new context which includes changes which were already made to state variables via SetState in the parent script.

@Tkael If this is the intended behaviour, then I can confirm that it does not work like this in beta 3. Setting the State() in Star scanned, and then using it in Star report, leads to the state. containing the value previous to the one set in Star scanned. See my first reply above with an example of what I mean. The F() function is not using the changes made, but retaining any value stored before the calling event script was run.

It's not just Star scanned / Star report either, I've found this to be the behaviour in other events/scripts too. Body scanned and Body report are prominent examples of the same issue.

EDIT: I've just run another little experiment. I set a variable to SecondsSince(0), read it out, set a State() to it, then call another script using F(). The other script simply reads out the state., and this does work as intended.

So, if a simple check like this works, but the event scripts do not, is there some problem with EDDI executing events that prevents them correctly setting/using state.?

EDIT2: OK, so I've tested the default personality, and weirdly, it works as expected. Going back to my personality, and it doesn't work, yet both are functionally identical?

EDIT3: Ohh, wait, I think I've figured it out after a couple more tests. It seems that EDDI is getting confused if there is more code AFTER the F(). So, in the default the F() is the last thing in the main script. In mine (and I assume in nepomuk's) there is additional code afterwards. If I remove that additional code, then it works as expected. Put the code back and it fails again.

So, it seems that F() only uses the set State() if, and ONLY if, it is the last thing in the main calling script!

Darkcyde13 commented 1 month ago

Too many edits on the above, so I'm starting a new comment. :)

So after even more tests, I'm not so sure about the 'being last' thing. In Star scanned it is already the last thing, in both mine and the default.

I tried deleting sections of code from mine, and retesting after each deletion. Only when almost all code was removed, did it work as expected, yet none of my code touches the State() variables. It's just speech with a couple of other SetState()s. I tried deleting different sections, in different orders, but nothing made it work sooner. it was always when practically all code was removed did it start to work.

This is my Star scanned script. Plugging this into a copy of the EDDI default also fails. So, what is it about this script that is preventing the State variables from working correctly?


{_ Context _}
{SetState('eddi_context_last_subject', 'star')}
{SetState('eddi_context_last_action', 'scan')}
{SetState('eddi_context_star_star', event.bodyname)}
{SetState('eddi_context_last_scan_estimated_value', event.estimatedvalue)}

{if ship.Role.invariantName = "Exploration" || ship.Role.invariantName = "Multipurpose":
    {set reportSystem to SystemDetails(system.systemname)}

    {if event.scantype = "AutoScan" && (SecondsSince(0) - state.eddi_context_autoscantime > 8):
        Auto scan complete.
        {SetState('eddi_context_autoscantime', SecondsSince(0))}
        {set autoscan to true}
    }

    {if event.mainstar:
        {if event.alreadydiscovered = false:
            Universal Cartographics {OneOf("has confirmed", "confirms", "has verified", "verifies")}
            you are the first {OneOf("", "commander", "person")}
            to {OneOf("discover", "enter", "visit")}
            {OneOf(P(system.systemname, "starsystem"), "this system")}.
        |else:
            {if state.eddi_context_undiscovered = true:
                Universal Cartographics
                {OneOf("database", "information")}
                {Occasionally(2, "has been")}
                {OneOf("refreshed", "updated")}.
                This system has {OneOf("already been discovered", "been discovered already")}.
                {set already_discovered to true}
            }
        }
        {SetState('eddi_context_undiscovered', false)}
    }

    {F("Star report")}
}

EDIT: Oh! Oh! I think I found it!! I removed the state. variables from the two checks that use them, and it works! I replaced state.eddi_context_autoscantime with 1, and state.eddi_context_undiscovered with TRUE, and it all works!

So, it seems if there is a use of state., that it seems to prevent SetState() from being used properly! The default Star scanned doesn't use any state.. Likewise, nor does the default Body scanned, but BOTH do in my personality! I tried setting each of those to the same value (1 and TRUE), and it still fails, but replace the actual state. with the value, and it works!

So, @Tkael there does appear to be a problem, just not the one I originally thought. It seems that retrieving a value set with SetState in the same script/context is dependant on if there are other uses of state. in the same script. Hope that can help fix it! :)

Darkcyde13 commented 1 month ago

OK, just ran my simple test again, using the below:

time set

{set test to SecondsSince(0)}
{test}
{SetState('timetest', test)}

{F('time test')}

{state.eddi_context_test}

time test

{state.timetest}

Run time set and you get two different values. Remove the last state. line from time set and you get both values being the same, as should be expected.

EDIT: It doesn't matter if state.eddi_context_test is populated or not, or if it is the first or last line in the script, the two values are always different when that state line is present, and the same when it is not present.

nepomuk16321 commented 1 month ago

Wow, that reads pretty complicated but also exciting. Unfortunately, I don't have time to contribute a few tests at the moment. I'm curious to see what @Tkael has to say.

Tkael commented 1 month ago

Thank you for the sleuthing and test scripts. Essentially it looks like I over-complicated state inheritance in the child script. Should be fixed in the next release. o7