-
Notifications
You must be signed in to change notification settings - Fork 24
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
Detecting invalid forms in lua #62
Comments
A possible fix: JCToLuaValue JCToLuaValue_fromItem(const item& itm) {
...
void operator ()(const form_ref& val) {
auto formId = (FormIdUnredlying)val.get();
if (!formid)
value = JCToLuaValue_None();
else
value.form = { (FormIdUnredlying)formId };
} |
That can do... but can't be also compared with Form(0) or something after retrieval? |
Haven't tried that. Will do as soon as I get home. |
@ryobg yeah, Form(0) should work as well |
@RealAntithesis any news here, all good? |
I'm doing more testing. For some reason, some of my form equality testing in lua doesn't appear to be giving consistent results. e.g. an array of forms testing against an array of objects that contain forms seems to be giving different results every time I test them. I need a more controlled test to see what's going on. It's clear though that 'printing' out both the array of forms and JMap of objects containing forms via papyrus, that the forms actually do appear in both but are equating to 'false' in the above test. |
Is it possible to get the formID from a form in lua? That may help workaround this since a formID is just a number that can be directly compared between lua and papyrus. |
@RealAntithesis , Lua Form has an |
Great, thanks. Will look into it. |
I've had a look into it. It appears that theForm.___id contains the correct formID for most forms, except for what looks like crafted potions and possibly forms originating from ESLs and ESPFEs (I need to verify this more). Plugging these formIDs into papyrus and using Game.GetFormEx(formID) returns none. i.e. theForm.___id returns a different number for some forms than theForm.GetFormID(). |
Do you use that ESL patched release from here (GitHub)? |
I thought I was. Will double check. |
Yep, this is with version 4.1.10 with the ESL fix. Besides, I ran the test concurrently in lua and papyrus at the same time, printing out the same array of forms. Papyrus successfully provided the formID when using theForm.GetFormID(), so the form itself was valid. The problem was that lua returned a different id from theForm.___id for some forms only.
Note that the form AC00332E originated from the Prvtl_HeavyArmory.esp plugin with no overrides in the load order, and which was a normal esp (no ESL flag). |
I think I figured it out. |
(scrathes head) The test is not okay? Depending on how you convert the decimals you may get these FFFF in front or you may not. Could be that the two number you compare are of differnt sign. One is signed, the other not. This will fail until you compare them bitwise say or convert them safely to proper type. |
The numbers are still coming out wrong though. I suppose I can pad the .___id number in lua so it matches the number in papyrus (edit, or chop out the FFFF FFFF in the papyrus number), but shouldn't they equal each other for the same form rather than converting the lua number to enable an equality test with the papyrus-generated number? Edit: the papyrus number is an Int. Lua doesn't appear to have a type? |
I don't know how this affects my other issue of forms not equalling each other. All I think I'm doing in that other test is to test the CData Form1 == Form2 (I'm not sure what this is doing in the background - is it just comparing the .___id numbers, or is there another comparison going on?). I'll have a another look at it. |
There may be a 32bit vs 24bit issue here? I tried padding out the .___id number to get to the formID generated by papyrus using BitOr(0xFFFFFFFF00000000,0xAC00332E), but instead got FFFF FFFF FFF0 332E (BitOr is my local lua function for bit.bor). However, BitOr(0xFFFF00000000,0xAC00332E) returned the same decimal number as papyrus, so I can use this to compare the id numbers. |
That Lua does not have integeres, and everything above the 24-bit range starts to mess up, if not an extended type. Check what types you are using here & there. Maybe it will be just easier to do what SilverIce suggested, and zero out the forms at the beginning. I'm not sure what I see in your investigation. |
Yep, grabbing the first 16 bits in the papyrus-derived formID is probably the way I'll go considering this is just a lua issue (I'm assuming the 2nd 16 bits will always be filled with FFFF FFFF and won't ever be anything else). |
I will suggest that you add a check for that assumption. Then let me know how this turned out and whether there is something which can be fixed. |
That's not true. _CForm is a structure containing uint32 ___id field.
Lua has doubles. The things mess up currently (#40) on C++ side due to Lua double -> Papyrus or JC float conversion. Would be cool to switch JC to use doubles. Or detect at least, that double value has no fraction numbers and convert it into Int on JC side
How did you got that value? Papyrus Int has 64 bits? |
I have to admit, I'm somewhat at a loss here. I've just done a test to compare forms in an array of forms against each other, printing out the results (and the formID being tested), and iterating this 6 times for each form. The intent of this was to test my earlier observation of getting different results when comparing forms in an array, but using more controlled conditions. In this test, the array of forms remained static (it was populated once in papyrus, and not changed in lua). The results seem to confirm my earlier observations that something incorrect is indeed happening - the search results for the same forms do appear to change over repeated iterations and the form values themselves (or at least their formIDs) appear to change without being explicitly altered by the user, even over a limited set of 6 observations. These are some of the results I'm getting (1st value is the form.___id field; 2nd value the hex of the form.___id field); 3rd value is the search result of the entire array to find the form - this should always be true since the form is taken from that same array; repeated 6 times):
Bad result (formID has changed for some reason, despite being the same form):
Bad result (search result has changed for some reason, but the form and array being searched hasn't changed):
Bad result (both formID and search result have changed during the 6 iterations, but neither the form nor the array being searched have changed):
Lua code: this is self contained, except for the papyrus code that populated the array (and retained it) and passed this through to lua in the objInventoryFormTestArray parameter, and the LogMessage() function which just prints to the log. In this test, there are 224 forms in the array.
|
There may also be something else going on. Sometimes on loading a save, some objects are empty and show up as nil when accessed (so lots of errors in the console about lua indexing nil objects etc), but the issue goes away when reloading the save again (nothing else changed except reloading the save). This doesn't happen very often though. I don't remember this being an issue on earlier versions of Skyrim/JC. |
You mean it was better with the previous, non-ESL patched version? |
I was thinking pre SSE 1.5.80. I'm going to revert back to the earlier version this week and see if similar issues are happening. |
Reverted the exe and SKSE (2.0.15) (and JC 4.1.8) back to 1.5.73 and it still happens. As an even simpler test, I ran the following in lua:
All this does is to access the form variable's .___id field over several iterations and report on the number of times it doesn't match the id picked up prior to the iterations. This works fine over 1000 iterations, until the line "formVar = objInventoryFormTestArray[1]" is added. Then it starts detecting errors - i.e. getting the .___id from the form is somehow resulting in a different value from the value at the start. But this only starts happening when the array is accessed in a nested loop. Maybe this is symptomatic of something else happening, but it's a bit weird. I tried a nest loop of incrementing a number, and that was fine. Accessing the array without a nested loop with the same line "formVar = objInventoryFormTestArray[1]" was fine. But putting this line into the loop suddenly resulted in the .___id field changing for seemingly random forms in the array (different forms would report errors over different runs in the same game session). Does this point to some sort of cause (memory issue etc)? |
Another test. In this one, I copy all the forms in the JArray to a lua table, and then compare the .___id fields. As expected by now, some compare the same, other don't. Over 3 iterations in the same game session, focusing on one particular index of both arrays, I got the following (the results are from comparing equality for the form and the other for the .___id field:
The code to copy the JArray to a lua table is fairly simple and direct:
|
@RealAntithesis , i'm lost in assumptions. Any chance the array is modified by another Papyrus thread while iterating through values? If so, can you deepcopy and pass the copy into lua via |
It's JC feature to not allow Lua to have state, since I don't save Lua state. |
Do you mean accessing the value using JValue.solvePath(optr, path) in jc.lua rather than through a local variable? The JMap objItemFormCache in the above example was already added to the JDB in papyrus (so should be retained). The intent of the above code was just to access this JMap directly within lua without having to pass it through via the function that uses it. |
There is a
You can't rely on it. Lua engine isn't thread-safe. There were 2 ways to solve that - have a single engine, protected by lock and have potential performance issues when more users use Lua (back then I though that was possible). The other (current) way was to have a pool of Lua engines. When 2 Papyrus threads call Other reason why in JC its best for Lua to have no state (no global variables) - users will expect that the state will be saveable |
If the objects are in JDB already, you can local objItemFormCache, objItemIDCache = JDB.AH_key.key1, JDB.AH_key.key2 Since this code executed in main body of your init.lua file, all Lua engines/contexts will run it, will have these values initialized |
Yep, that's what I've started doing:
The only issue is what to do with 'bDebugScript' and 'debugLevels' - these weren't originally in the JDB. I've put them all into the JDB and it seems fine now. |
Just going back to the original question raised in this thread re: testing for invalid forms in lua, testing for invalid forms using Form(0) doesn't appear to work. A formID may become invalid due to a change in load order, so Game.GetFormEx(formID) may give a nil form. However, in this case, testing the now invalid form in lua against Form(0) still results in false, meaning it tests as valid (not equal to Form(0)) when it is not. Would the other method suggested at the top of this thread yield a different result? |
I don't think so. The solution appears the same. When the form is created it is created as invalid (kinda |
Thanks - doing it in papyrus seems to work fine for now. I was just wondering if the discussion in the other thread about resolving forms and formIDs using the SKSE function would be relevant here since that's what papyrus GetFormEx() would be doing (which works fine). |
That resolving is used only during loading. The retain/release... well you know. These where the only changes. Or you mean like exposing for calling a function? |
More that there should be a way to determine that a form is no longer valid if the load order changes - so if, for example, a Circlet of Archery in one game session has a formID of 1032235, but then the user changes mods around which causes the formID to change in the next game session to 1694524400, presumably the previous formID should no longer resolve to a valid form? If this is detected, then the form should revert to something like nil or Form(0). At the moment, I can't figure out a way to determine that the form is invalid since it still seems to have the old formID (.___id) and otherwise seems normal in lua and doesn't equal Form(0) - but it does read as zero in papyrus (i.e. the form actually is invalid). |
Then I guess some SKSE function can do that, have to see which one. If you use say |
I'll test that out. I suppose I was assuming that jcontainers would be rebuilding the forms on start up anyway, since the form ID should just be the base form ID (00xxxxxx for normal ESPs and FE000xxx for ESLs and ESPFEs) with the first 2 digits being the plugin load order number for ESPs and 3rd to 5th digit for ESLs and ESPFEs. Then, the only thing needing to be done on load should be detecting if the originating mod's load order has changed, and then changing the load order digits to match. As a bonus, if the mod has been removed from the load order or the form removed from the mod, it should return nil for the form (and then reconstituting the form if the mod is added back into the load order). I think I can do all this from papyrus, it will just be a little slower (I'll need to get a list of all loaded mods using the SKSE function, grab the load order digits from the form ID, store the mod name, load order index and base form IDs separately, detect invalid forms in the next load, and then rebuild the form ID from the new mod load order index and base form ID and get the form for the new form ID). Should work... |
Why do all of this, if JC already fixes form ID during save file load? |
I suppose that's the thing. I'm coming across invalid forms resulting from changing the load order (adding or removing mods that otherwise have nothing to do with those forms). So part of my question was whether JContainers was supposed to be dealing with that anyway. |
Hmm, hold fire on this. It may be something I'm doing. Let me test things out further. |
@RealAntithesis |
Everything works now, so I'm going ahead with the AH Hotkeys update. The issue immediately above is that I had absentmindedly switched to using formIDs rather than forms (due to the earlier esl issue), so of course they don't update when the load order changes (duh). Switching back to using the form map is working fine now. Is the next version of JContainers due to be released soon, including the fix to the form comparison issue in JC.Lua (I've already made this change in my version, but nobody else would have it)? Thanks. |
Can you tell me now, after 54 comments, is there something to fix here or not? I was delaying that ESL fix release due to your two questions. |
The only outstanding question was the testing for an invalid form, but I think that had been largely resolved with both the esl fix and the form comparison fix discussed above. I think we should be able to go ahead with the release unless you think there is something further that needs looking at in the code based on the above. Thanks. |
So far I see like 3 possible improvements (all proposed from @SilverIce):
|
#1 I think was resolved with the change in jc.lua? |
|
I can test a test release if you make one. |
Well... it will take much more time. Apparently with that fix proposed all Lua tests broke. I have to research how this whole thing work before even proposing a change. |
@ryobg IDK, I had zero issues with Lua CForm fix: SilverIce@255f512 |
@SilverIce It's about #40 - allowing 32-bit integers. I tried that 1st fix #40 (comment) and it broke. |
Is there a way to determine whether a form is invalid in lua? When papyrus accesses an invalid JC form (e.g. forms have changed due to a different load order etc), it reads as nil. But that form still appears to have a value (CData) in lua - so testing for nil doesn't work. Essentially, I want to be able to delete invalid forms (or otherwise act on them) from a lua script. Thanks.
The text was updated successfully, but these errors were encountered: