ryobg / JContainers

JSON-based data structures for Papyrus - the TESV Skyrim SE scripting language
MIT License
107 stars 23 forks source link

Add JArray to Papyrus array functionality #28

Closed ryobg closed 6 years ago

ryobg commented 6 years ago

Currently there is this Papyrus function from JC:

Form function getForm(Int object, Int index, Form default=None) global native

Which can be used also to walk an array and get at the end Form[] type.

The idea is to instead have a:

Form[] function getForm(Int object, Form default=None) global native

That is, faster way to convert the JC array into native, Papyrus one.

See https://forums.nexusmods.com/index.php?/topic/6560841-jcontainers-se/?p=59903116

ryobg commented 6 years ago

I have built test version here

Added functions are

Int[] function allInt(Int object) global native
Float[] function allFlt(Int object) global native
String[] function allStr(Int object) global native
Form[] function allForm(Int object) global native

(I have troubles adding one for the allObj variant)

I think these should work, but it is blind shot, as I do not have working Skyrim installation - I'm trying to save what I have, but this is big task which I'm yet hesitant to take...

aliendiplomat commented 6 years ago

Outstanding. Will download and and do some timing trials ASAP!

EDIT: Nice, you even added it to Domain context. :)

ryobg commented 6 years ago

lol - paladin Danse

Let me know when you have tested them 👍

aliendiplomat commented 6 years ago

I did some preliminary testing...

; first i create a new JArray from a file and tell it to retain:

int testJCArray = JValue_readFromFile("Data/SKSE/Plugins/JCData/Domains/phiASDomain/testJCArray.json")
JValue_retain(testJCArray, "phiASFilters")

; printing the size verifies the data is loaded at this stage

int count = JArray_count(testJCArray)
Debug.Notification("JArray Size = " + count); shows correct count

; test the new function:

Form[] tForms = JArray_allForm(testJCArray)
int tl = tForms.Length
Debug.Notification("Length = " + tl); so far so good, shows correct number...

; contents of "Data/SKSE/Plugins/JCData/Domains/phiASDomain/testJCArray.json":

[
  "__formData|Skyrim.esm|0x2e504",
  "__formData|Skyrim.esm|0x2e4f3",
  "__formData|Skyrim.esm|0x2e4ff",
  "__formData|Skyrim.esm|0x2e4fb",
  "__formData|Skyrim.esm|0x2e4e5",
  "__formData|Skyrim.esm|0x2e4e3"
]

EDIT: OK, my bad. I used "length" for the name of my int variable for checking the array length and apparently that is a protected expression (derp). Changed it to "tl" and it compiles and prints the correct size of the array in-game.

Moving forward with testing...

EDIT EDIT: SUCCESS!

I was able to convert the array to a FormList for use in mass container sorting. Only tested the Form version so far, and with array sizes of only up to a couple hundred items, however it does appear very fast. Will have to see about pushing the limits but so far so good!

ryobg commented 6 years ago

Congratulations ^.^

I will wait a bit more for further feedback. If all fine, I can go with the release.

P.S. Out of curiosity how is that "sorting" done? In Papyrus, by custom criteria or?

SilverIce commented 6 years ago

(I have troubles adding one for the allObj variant)

This is a bad idea. At least users of allObj had to be aware of the fact, that these object handlers aren't temporarily-retained.

On regards of function naming. Would be cool to not use short function names, as I did it in past. From the API design perspective - long function names will be more informative for users. Like: asIntegerArray, asStringArray From Papyrus point of view: no conflicts between function names (in future) From the user point of view: a string can't be read as Int, you may want to add default values like

Int[] asIntegerArray(int object, int defaultValue=0)
ryobg commented 6 years ago

Neither the values from get* are retained. It makes sense to add a note about this.

I will look forward how to support allObj.

I like the idea for better names. I decided to go the old way just to be consistent.

In the current code for get* the default value passed as parameter is only used when the read is not reached i.e. I did not saw an use of it. If the container is heterogenous the read already include default values - this is consistent with the current behaviour of get* for which these new all* are mere wrappers.

Will consider - thank you for the input.

aliendiplomat commented 6 years ago

Hey guys, thanks a lot for continuing to work on this. :)

Did some further testing and everything looks good. Basically, I used the new functions to convert a JC array of 50 of each type (int, float, string, form) to a native Skyrim arrays. Then I converted those back into new JArrays and output the contents to compare. Values are as expected.

Here is the simple test script I used. All you have to do is load up any saved game and it will perform the conversion and output the files for comparison: TestFunctions.zip

Not sure what additional testing might be helpful. Could do the same thing with input arrays of hundreds of values if that would be of any use, but integrity checks out and the functions are definitely fast, enough for my own modding purposes to be sure. :)

ryobg commented 6 years ago

Thank you. These days I will release.

RealAntithesis commented 6 years ago

I think I'll definitely have a use for this. So, is the difference between these new array functions vs the already-existing "writeToFormPArray" etc functions that the new functions will create the array of the required length without being created with a fixed size the vanilla-papyrus way? Also, will this create arrays greater than 128 elements?

ryobg commented 6 years ago

@RealAntithesis,

writeToForm*Array provide fine grain control. As for these new functions, array size is taken care of in JC, so no need to provide length. I also do not see issues with bigger arrays.

ryobg commented 6 years ago

Ok, I have left out the asObjArray functions as a bit tricky. Functions renamed to follow the as*Array template. Comments extended. So this should do, for upcoming release.

If you have any other remarks let me know.