Author Topic: Continuing Karajorma's work on SEXP containers  (Read 3151 times)

0 Members and 1 Guest are viewing this topic.

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Continuing Karajorma's work on SEXP containers
I'm trying my hand at continuing (and hopefully finishing) Kara's SEXP containers feature.

I'm working off this branch (commit log), which is based on a rebased version of Kara's WIP PR. I'll put up a PR of my own later.

Links to current builds:

https://joshuaglatt.com/fso/fs2_open_21_1_0_x64_AVX2-FASTDBG.7z
https://joshuaglatt.com/fso/fred2_open_21_1_0_x64_AVX2-FASTDBG.7z


EDIT: Original threads for reference:
https://www.hard-light.net/forums/index.php?topic=88328.0
https://www.hard-light.net/forums/index.php?topic=88613.0
« Last Edit: April 21, 2021, 11:42:10 pm by jg18 »

 

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Re: Continuing Karajorma's work on SEXP containers
Fixed a crash Karajorma reported. The list containers were being populated incorrectly. I've updated the posted .zip files and the GH branch.

 

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Re: Continuing Karajorma's work on SEXP containers
Finished reading through the code.lib changes. While there are various bits of cleanup, polishing, and simple perf improvements I'd like to do, I didn't see any obvious bugs.

Review thoughts:
  • The Ctext_strings hack in sexp.cpp needs to go. We're storing C-strings constructed via CTEXT() in an array with 🤞 that the strings will still be there whenever we need them. :nono: How about instead adding a char[] of size TOKEN_LENGTH in the sexp_cached_data struct and then storing the C-string in the node's cache field?
  • Since we want to be able to delete entries at any position in a list container, SCP_list is the right data structure and not SCP_deque. I'll fix it later.
  • What is the point of using container-data-index with a map container? Why would a FREDder do this?
  • If a list has duplicate entries, remove-from-list will remove only the first instance. Is that the desired behavior?
  • FREDders should be cautioned that some container operations are slow, pretty much anything that requires iterating through a list container, due to pointer chasing from node to node. But maybe the perf hit isn't noticeable for short lists? 🤷‍♂️

Potentially useful extra features to consider:
  • shuffle-list SEXP to randomly shuffle the contents of a list container. We'd have to copy the contents into an SCP_vector, shuffle the vector, and copy the result back into the list, so quite expensive.
  • get-map-value SEXP to retrieve a value from a  map, given a key; maybe also with an optional extra arg for the name of a variable to store the retrieved value
  • Array container type (SCP_vector internally; details below)

Advantages of array containers:
- get_random and at_index take constant time
- random shuffling is more efficient, no copying required
- getting the next array entry is just incrementing a pointer, ultra fast, as opposed to in a list, which requires pointer chasing (slow)

Disadvantages are, well, the usual issues with arrays, like deleting at anywhere other than the end being crazy expensive; perhaps those operations should be disallowed for arrays.

Thoughts?

  

Offline karajorma

  • King Louie - Jungle VIP
  • Administrator
  • 214
    • Karajorma's Freespace FAQ
Re: Continuing Karajorma's work on SEXP containers
When it came to speed issues, I barely considered them since I figured that most FREDders are going to making missions that result in one container with maybe 20 items at most being altered once a second (and probably a lot less often than that). I didn't expect this feature to ever have containers large enough that it would ever cause much of a speed hit. That said, if there are ways to be more efficient, I'm happy to explore them.

2) I used deque cause I wanted the FREDder to be able to remove first or last. I expect those two to be the most common used. But it's possible that assumption is wrong.

3) I'm at work at the moment but I suspect that they wouldn't have much use for that one. But I'll double check once I'm home.

4) Yes, that is absolutely intentional. I wanted to make it so that FREDders can make certain outputs more likely by stuffing the container with more than one instance same as they currently can with argument lists. This is especially useful when the player is using a multidimensional container. That said, if we could make it optional to remove other instances that's not a bad idea. It should default to not doing that though since that is much faster.
Karajorma's Freespace FAQ. It's almost like asking me yourself.

[ Diaspora ] - [ Seeds Of Rebellion ] - [ Mind Games ]

 

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Re: Continuing Karajorma's work on SEXP containers
When it came to speed issues, I barely considered them since I figured that most FREDders are going to making missions that result in one container with maybe 20 items at most being altered once a second (and probably a lot less often than that). I didn't expect this feature to ever have containers large enough that it would ever cause much of a speed hit. That said, if there are ways to be more efficient, I'm happy to explore them.
Fair points, and I don't want to prematurely optimize. Also agree that containers seem unlikely to be a performance bottleneck. The changes I had in mind were more about making the code more compact and easier to read. We'll see what you and the rest of the team think when I have some of those changes up.

2) I used deque cause I wanted the FREDder to be able to remove first or last. I expect those two to be the most common used. But it's possible that assumption is wrong.
A list can add/remove items at the start/end in constant time.

http://www.cplusplus.com/reference/list/list/emplace_front/
http://www.cplusplus.com/reference/list/list/emplace_back/

http://www.cplusplus.com/reference/list/list/pop_front/
http://www.cplusplus.com/reference/list/list/pop_back/

A list can remove an entry in the middle in constant time, but finding the entry of course takes linear time.

4) Yes, that is absolutely intentional. I wanted to make it so that FREDders can make certain outputs more likely by stuffing the container with more than one instance same as they currently can with argument lists. This is especially useful when the player is using a multidimensional container. That said, if we could make it optional to remove other instances that's not a bad idea. It should default to not doing that though since that is much faster.
Ok, good to know. We'll see whether we can squeeze in a "remove all" option.


Also, forgot to mention before: I think "Strongly Typed" should be renamed "Strictly Typed", since that term will more easily make sense to non-coder FREDders. But, since who knows when that feature will get added, maybe not super important.

 

Offline karajorma

  • King Louie - Jungle VIP
  • Administrator
  • 214
    • Karajorma's Freespace FAQ
Re: Continuing Karajorma's work on SEXP containers
I'm fine with the naming change to strictly typed. I think it would be easier for them to understand too.
Karajorma's Freespace FAQ. It's almost like asking me yourself.

[ Diaspora ] - [ Seeds Of Rebellion ] - [ Mind Games ]

 

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Re: Continuing Karajorma's work on SEXP containers
Update: I now have the FRED support working (experimental). As a sanity check, I was able to view the containers that are in the test mission Kara shared in #scp-private on Discord.

Builds in the OP have been updated. Each .7z file includes the .exe and .pdb files.

I might be interested in some changes to the added UI for FRED, most notably making a container's type and its data(/key) type fixed at container creation time and not editable after that. However, I'll be sure to run any proposed UI/UX changes by Kara before officially making them. :)

 

Offline karajorma

  • King Louie - Jungle VIP
  • Administrator
  • 214
    • Karajorma's Freespace FAQ
Re: Continuing Karajorma's work on SEXP containers
I might be interested in some changes to the added UI for FRED, most notably making a container's type and its data(/key) type fixed at container creation time and not editable after that. However, I'll be sure to run any proposed UI/UX changes by Kara before officially making them. :)

I'm surprised I left that as editable. I suppose it was just because I figured that FREDders would pick the wrong one initially. If I had a pound for every time I've made a variable and got an error message cause I forgot to click on the String option......
Karajorma's Freespace FAQ. It's almost like asking me yourself.

[ Diaspora ] - [ Seeds Of Rebellion ] - [ Mind Games ]

 

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Re: Continuing Karajorma's work on SEXP containers
I might be interested in some changes to the added UI for FRED, most notably making a container's type and its data(/key) type fixed at container creation time and not editable after that. However, I'll be sure to run any proposed UI/UX changes by Kara before officially making them. :)

I'm surprised I left that as editable. I suppose it was just because I figured that FREDders would pick the wrong one initially. If I had a pound for every time I've made a variable and got an error message cause I forgot to click on the String option......
Hmm, on second thought, I propose the following:
- A new container is always initialized as being a list of string data.
- Only empty containers have editable type and data(/key) types.
- When a non-empty container is selected, or when there are no containers, the various type radio buttons are disabled.
- When the FREDder inserts/adds/updates a container entry, FRED will check the type of the new/modified entry and reject the entry if the type check fails. (Exactly what "reject" will look like is TBD.)

Thoughts?

 

Offline karajorma

  • King Louie - Jungle VIP
  • Administrator
  • 214
    • Karajorma's Freespace FAQ
Re: Continuing Karajorma's work on SEXP containers
Yeah. That makes sense.
Karajorma's Freespace FAQ. It's almost like asking me yourself.

[ Diaspora ] - [ Seeds Of Rebellion ] - [ Mind Games ]

 

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Re: Continuing Karajorma's work on SEXP containers
Cool. Also, re: the "number" type for container data (and/or map keys), what does "number" mean? Positive integer? Non-negative integer? Any integer? Something else?

 

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Re: Continuing Karajorma's work on SEXP containers
Oh also, suggested constraints on valid container names:
- ASCII chars (letters/numbers/printable symbols) only
- Not case-sensitive
- No spaces in the name allowed (EDIT: I could bend on this one, as long as FSO/FRED handles names with spaces fine.)
- No leading/trailing whitespace
- Cannot contain & (already enforced)
- Must start with a letter (probably not strictly necessary, but would likely make things easier)

Sound ok?
« Last Edit: March 13, 2021, 11:29:25 pm by jg18 »

 

Offline karajorma

  • King Louie - Jungle VIP
  • Administrator
  • 214
    • Karajorma's Freespace FAQ
Re: Continuing Karajorma's work on SEXP containers
Cool. Also, re: the "number" type for container data (and/or map keys), what does "number" mean? Positive integer? Non-negative integer? Any integer? Something else?

I'd stick to any integer since I'm pretty sure that SEXP variables can also use those.

Oh also, suggested constraints on valid container names:
- ASCII chars (letters/numbers/printable symbols) only
- Not case-sensitive
- No spaces in the name allowed (EDIT: I could bend on this one, as long as FSO/FRED handles names with spaces fine.)
- No leading/trailing whitespace
- Cannot contain & (already enforced)
- Must start with a letter (probably not strictly necessary, but would likely make things easier)

Sound ok?

Sounds good to me.
Karajorma's Freespace FAQ. It's almost like asking me yourself.

[ Diaspora ] - [ Seeds Of Rebellion ] - [ Mind Games ]

 

Offline karajorma

  • King Louie - Jungle VIP
  • Administrator
  • 214
    • Karajorma's Freespace FAQ
Re: Continuing Karajorma's work on SEXP containers
I've been getting a lot of crashes with this build. I posted the errors on the Discord. I've attached the mission. If you attempt to change the message in the mission as shown in the pictures you should be able to reproduce the crash.


EDIT: Found another crash. If I create a new container and attempt to use it without saving first FRED crashes out with the assertion in the picture below as soon as I right click on Get First. If I save, everything is fine. I'd attach a mission displaying the error but the act of saving it prevents the error occurring.
« Last Edit: March 21, 2021, 09:23:35 am by karajorma »
Karajorma's Freespace FAQ. It's almost like asking me yourself.

[ Diaspora ] - [ Seeds Of Rebellion ] - [ Mind Games ]

 

Offline karajorma

  • King Louie - Jungle VIP
  • Administrator
  • 214
    • Karajorma's Freespace FAQ
Re: Continuing Karajorma's work on SEXP containers
Okay so let me post this stuff from the Discord.

Run the mission attached and press 2. The mission should go then send a message from a random ship. It choose the message by going into the map called Traitors and looking for a key called <argument> (which at random will be cancer, arise or taurus), Those keys match map containers with the same names. The game is apparently doing this check correctly, but failing to check the Cancer, Aries or Taurus maps for the data that corresponds to the key MESSAGE-Treason. Instead it appears to be checking the Traitors map again.

I can't say what the cause of the issue is but I'm fairly certain what is going on. The call to deal_with_container_sub() is being given the container_index for Traitors rather than the index for Cancer, Aries or Taurus.
Karajorma's Freespace FAQ. It's almost like asking me yourself.

[ Diaspora ] - [ Seeds Of Rebellion ] - [ Mind Games ]

 

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Re: Continuing Karajorma's work on SEXP containers
Hmm, so it sounds like there are three unrelated bugs here:
1) Crash in FRED when trying to use a list modifier in the SEXP tree in Events Editor.
2) Using a new container in FRED's Events Editor without saving the mission first causes a crash.
3) Multidimensionality (parsing the result from a container lookup/operation as a reference into another container) is broken.

For 1 and 2, I don't understand your steps to repro. I think there are some unstated/implied actions/steps that I'm not aware of. Keep in mind that I haven't used FRED since 2012. I also don't know how to use the Containers feature in FRED; I just know some things about how containers work under the hood.

As such, can you please provide more thorough steps to reproduce for 1 and 2?

I think I understand the repro steps for 3, but a more thorough list of steps wouldn't be a bad idea.

Re: bug 1, I noticed some code that got lost in the rebase in sexp_tree that's related to assigning operators, so maybe fixing that will help fix this.

Re: bug 2, this might be fixed now that I removed the "raw_data" thing from the edit containers dialog. All edits made in the dialog UI now affect the edit_sexp_containers variable (now called m_containers) directly. The edited containers still aren't saved to the global Sexp_containers until the user clicks OK, though.

I'd like to provide an updated build, but it's a bit tricky in that I'm still working out issues in my revised version of the edit containers dialog. As I mentioned on Discord, the UI isn't changed, mostly internals, plus a few small changes in user-facing behavior.

I'll see what fixes I have tonight and provide an experimental build that may or may not fix some of these things.

If you can provide more complete repro steps though that'd help a lot.

 

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Re: Continuing Karajorma's work on SEXP containers
Ok, with my latest changes, looks like multidimensionality is now fixed. When I run ContainersTest2 and press 2, I get a random "traitor" message, such as "You will all die".

Speaking of messages, FS2/FSO's built-in text-to-speech finally comes in handy. :D I don't have to pull up/squint at the message logs to find out what was just said. Neat!

Builds in OP updated. However, since the revised edit container dialog still needs more testing, I'm keeping the old builds here:

https://joshuaglatt.com/fso/old/fs2_open_21_1_0_x64_AVX2-FASTDBG.7z
https://joshuaglatt.com/fso/old/fred2_open_21_1_0_x64_AVX2-FASTDBG.7z

The other two issues probably aren't fixed yet, but who knows?


BTW, if we're open to switching to an RNG that resembles true randomness much better than rand(), I suggest PCG. I've used it in other projects and have been impressed with it. It's fast and uses very little space, and it's also header-only and under Apache License 2.0, so easy to include. :)

 

Offline karajorma

  • King Louie - Jungle VIP
  • Administrator
  • 214
    • Karajorma's Freespace FAQ
Re: Continuing Karajorma's work on SEXP containers
I'm okay with switching RNG. The first 4 times I ran the mission it picked 4 and I started to wonder if the function had broken somehow.
Karajorma's Freespace FAQ. It's almost like asking me yourself.

[ Diaspora ] - [ Seeds Of Rebellion ] - [ Mind Games ]

 

Offline karajorma

  • King Louie - Jungle VIP
  • Administrator
  • 214
    • Karajorma's Freespace FAQ
Re: Continuing Karajorma's work on SEXP containers
The first crash seems to be gone and multidimensionality also seems to be fixed. But trying to open the edit containers option on a brand new mission crashes FRED with the error

Code: [Select]
Assert: "m_current_container >= 0 && m_current_container < num_containers()"
File: editcontainerdlg.cpp
Line: 662

Simply open FRED, create a new mission, open the event editor, create an event and then choose Edit Containers from the menu to crash FRED.
Karajorma's Freespace FAQ. It's almost like asking me yourself.

[ Diaspora ] - [ Seeds Of Rebellion ] - [ Mind Games ]

 

Offline jg18

  • A very happy zod
  • 210
  • can do more than spellcheck
Re: Continuing Karajorma's work on SEXP containers
FRED crash fixed. The cause was just an obsolete assertion.

Builds in the OP updated.

The old builds from before my revisions to the edit container dialog are still up, though.

I also spent some time looking through FSO/FRED's use of rand() and srand() but that's a story for another thread. :)