Hard Light Productions Forums

Modding, Mission Design, and Coding => FS2 Open Coding - The Source Code Project (SCP) => Topic started by: jg18 on March 07, 2021, 01:22:16 am

Title: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 07, 2021, 01:22:16 am
I'm trying my hand at continuing (and hopefully finishing) Kara's SEXP containers feature.

I'm working off this branch (https://github.com/jg18/fs2open.github.com/tree/feature/sexp-containers-revised) (commit log (https://github.com/jg18/fs2open.github.com/commits/feature/sexp-containers-revised)), 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
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 07, 2021, 11:54:33 am
Fixed a crash Karajorma reported. The list containers were being populated incorrectly. I've updated the posted .zip files and the GH branch.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 07, 2021, 09:29:16 pm
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:

Potentially useful extra features to consider:

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?
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on March 07, 2021, 10:59:02 pm
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.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 07, 2021, 11:37:58 pm
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.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on March 08, 2021, 12:50:13 am
I'm fine with the naming change to strictly typed. I think it would be easier for them to understand too.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 13, 2021, 09:26:12 pm
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. :)
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on March 13, 2021, 09:37:40 pm
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......
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 13, 2021, 11:11:00 pm
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?
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on March 13, 2021, 11:12:17 pm
Yeah. That makes sense.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 13, 2021, 11:13:03 pm
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?
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 13, 2021, 11:22:06 pm
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?
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on March 14, 2021, 12:06:41 am
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.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on March 21, 2021, 09:02:53 am
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.


[attachment deleted to save space]
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on March 21, 2021, 11:21:48 am
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.

[attachment deleted to save space]
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 21, 2021, 05:02:38 pm
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.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 22, 2021, 01:13:13 am
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 (https://github.com/imneme/pcg-cpp). 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. :)
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on March 22, 2021, 07:01:51 am
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.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on March 22, 2021, 09:40:52 am
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.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 23, 2021, 01:14:49 am
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. :)
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on March 28, 2021, 03:03:02 am
Not a bug as such but something which could potentially become annoying for mission designers. Press 3 in the attached mission. The game will send a message which contains this syntax &TestMapContainers&Key 2&

The first time it does this, TestMapContainers exists and contains a key called Key 2. The mission then uses the remove-from-map SEXP to remove this entry and then sends the same message. The game then sends the following warning as expected.

Code: [Select]
Warning: sexp_replace_container_refs_with_values() found a container called TestMapContainers to replace but the modifer is not recognised
File: sexp.cpp
Line: 23938

However, clicking continue simply keeps bringing that message up again seeming forever. It really should only appear once and then bug out gracefully.

The problem seems to be that if the game can't replace the text it returns false immediately and lookHere is never updated. So the game keeps trying to replace the text in the same place instead of giving up and moving on.


EDIT: Line 710 seems to contain an error. It reads

Code: [Select]
{ "get-map-keys", OP_GET_MAP_KEYS, 2, 2, SEXP_ACTION_OPERATOR, }, // Karajormabut it should be
Code: [Select]
{ "get-map-keys", OP_GET_MAP_KEYS, 2, 3, SEXP_ACTION_OPERATOR, }, // KarajormaSo that you can use the optional argument.


EDIT 2: Similar to the first error. Sending a message which uses the contents of a container when the container is empty seems to also put the game into an infinite loop, this time with no error. I've attached a version of the mission that does that too. Press 4 to cause the error.

[attachment deleted to save space]
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on March 31, 2021, 11:46:23 pm
Still working on the two bugs in the last post, but wanted to clarify some requirements:

First, I think a few of the new SEXPs should have their names/behaviors reconsidered:


Thoughts?


Second, I'm unclear on which string comparisons are case-sensitive and which aren't.

From what I understand, these values are case-sensitive:

And these values are not case-sensitive:

Is this right? Am I missing anything?

Thanks.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 01, 2021, 08:17:43 am
First, I think a few of the new SEXPs should have their names/behaviors reconsidered:

  • "container-data-index" should be "list-data-index". Maps are unordered, so map keys/values don't have indices. Using this SEXP with a map should be an error.
  • "container-has-key" should be "map-has-key". Lists obviously don't have keys. Using this SEXP with a list should be an error.

I'm fine with both of those changes.

Quote
I also think "container-has-data" should be "list-has-data". I don't understand how it's useful to check whether a map's values include certain values. Even if you know that some specific value/data is in the map, you don't know its associated key and thus can't retrieve it.

I disagree with this one though. At the moment the easiest way to iterate through a map is to


That will take one frame per key in your map and it requires the overhead of all the other checks needed to make it work. The SEXP I said I'd code to allow the exporting of a list to an argument list will cut down some of that overhead, but you're still going to have a lot of overhead. And sometimes, if the user needs that key that is what the user will have to do.

But sometimes the user might not want the key in this particular event. They might only care if the data is in the map. For instance, a map of surviving wingmen might not care what the key is, only that a certain wingman is alive. We might not need to manipulate the data itself in any way because what we're doing is having another character react to their situation. In cases like that, it seems silly to force the FREDder to roll all that stuff themselves when the SEXP already supports finding that info.

What I might suggest is that we split the SEXP into two SEXPs and map-has-data takes a second argument which is a string variable where we store the key name. I would include a message in the notes for container-has-data to use map-has-data if you need the key name, but otherwise leave it the same.


Quote
Second, I'm unclear on which string comparisons are case-sensitive and which aren't.

From what I understand, these values are case-sensitive:
  • List data
  • Map keys

And these values are not case-sensitive:
  • Container names
  • List modifiers (EDIT: although it would make things easier if they were case-sensitive)

Is this right? Am I missing anything?

Thanks.

That sounds about right. Of course I'm open to comment on if anyone would prefer it if anything was changed.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 01, 2021, 10:26:33 pm
Re: "container-has-data" and maps, ok, that's fair and makes sense. Interesting idea about the optional arg for map-has-data.

To sum up, what if we add these SEXPs?

For "map-has-data-item", if multiple keys have the specified string as their data, which key is assigned to the variable (if provided) is arbitrary.

We'll then remove these SEXPs:

All other container SEXPs will remain unchanged.

The SEXP I said I'd code to allow the exporting of a list to an argument list [...]
Is that for-container? Or are you referring to something else?


Re: container names and list modifiers being case-insensitive, part of the reason I asked is that string comparisons for list modifiers are currently case-sensitive. I wasn't sure if that was intended behavior or a bug.

Can you create a test mission that includes checks that case-insensitive comparison works, both for SEXP usage and in text replacement and both for container names and for list modifiers?
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 02, 2021, 04:16:32 am
Re: "container-has-data" and maps, ok, that's fair and makes sense. Interesting idea about the optional arg for map-has-data.

To sum up, what if we add these SEXPs?
  • list-has-data = like "container-has-data" for lists; returns true only when all of the data items specified are in the list
  • list-data-index = like "container-data-index" for lists; takes only one data item and returns the index of first occurrence, or -1 if not found
  • map-has-keys = like "container-has-key" for maps; returns true only when all of the strings specified are keys in the map. (Will this work if the map has numeric keys?)
  • map-has-data-item = checks if a single data item is the value associated with some key in the map. As you suggested, it'll takes an optional arg of a string variable to store the key if the data is found. If the data is not found, the SEXP returns false and the string variable (if provided) is left unchanged. I guess we could require a number variable if the map has numeric data, but maybe that's getting too complicated.

For "map-has-data-item", if multiple keys have the specified string as their data, which key is assigned to the variable (if provided) is arbitrary.

We'll then remove these SEXPs:
  • container-has-data
  • container-data-index
  • container-has-key

All other container SEXPs will remain unchanged.


I'm fine with that. We should add a note to the SEXP description that says the key given by map-has-data-item will be random (and code the SEXP so that it is actually random, not simply the first one).

When it comes to map-has-key with numeric keys, a FREDder might want to use the return value from a numeric SEXP so we might need to think a little about how to set that one up.

Quote
Is that for-container? Or are you referring to something else?

It is. I couldn't remember what I had called it on Discord.

Quote
Re: container names and list modifiers being case-insensitive, part of the reason I asked is that string comparisons for list modifiers are currently case-sensitive. I wasn't sure if that was intended behavior or a bug.

Can you create a test mission that includes checks that case-insensitive comparison works, both for SEXP usage and in text replacement and both for container names and for list modifiers?

Sure, I'll see if I can do that tonight.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 02, 2021, 11:59:11 pm
We should add a note to the SEXP description that says the key given by map-has-data-item will be random (and code the SEXP so that it is actually random, not simply the first one).
Notice that I didn't say "random"; I said arbitrary, as in, you get whatever you get, namely the key for the first matching entry found. I'd rather not add the complexity (and processing overhead) of doing truly at random, unless there's a clear use case for it.

When it comes to map-has-key with numeric keys, a FREDder might want to use the return value from a numeric SEXP so we might need to think a little about how to set that one up.
Ok, sure. Can you create a test mission that uses map-has-key this way? We can then fine-tune the syntax to fit how a FREDder would actually want to use it.

Also, I noticed you said "map-has-key". Should the SEXP accept only one key? Or should it be able to check any number of keys?


Finally, can we have a single map-has-key (or map-has-keys) SEXP and a single map-has-data-item SEXP that can work with either string keys or numeric keys (and, in the case of map-has-data-item, either string data or numeric data)? Can FRED support that type of flexibility?
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 03, 2021, 11:34:43 pm
Notice that I didn't say "random"; I said arbitrary, as in, you get whatever you get, namely the key for the first matching entry found. I'd rather not add the complexity (and processing overhead) of doing truly at random, unless there's a clear use case for it.

I think it would be useful to be able to return a random key. But probably not useful enough that it justifies the overhead of doing it in every case. If making it truly random later gets enough requests we can always change it without breaking correctly made missions since we said that the key it gave you was arbitrary it's the FREDders own fault for assuming that it wouldn't change.

Quote
Ok, sure. Can you create a test mission that uses map-has-key this way? We can then fine-tune the syntax to fit how a FREDder would actually want to use it.

Come to think about it, the player could always just use the int-to-string SEXP so it's probably smarter to let them do that than to complicate things with another SEXP. We only really need the string version.

Quote
Also, I noticed you said "map-has-key". Should the SEXP accept only one key? Or should it be able to check any number of keys?

It should check any number of keys. It's just that FRED tends to use the singular whenever a SEXP takes one or more arguments string-equals, -is-destroyed-delay, etc. so I've gotten into the habit of naming SEXPs that way.

Quote
Can you create a test mission that includes checks that case-insensitive comparison works, both for SEXP usage and in text replacement and both for container names and for list modifiers?

Attached


By the way, the Edit Containers dialog won't let me create a container with a space in it but I'm pretty sure the code does support that. I've certainly had no issues so far with the containers I created before that restriction was added. Of course, if it makes things easier to limit containers in this way, I have no objection. 

[attachment deleted to save space]
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 04, 2021, 12:27:51 am
Come to think about it, the player could always just use the int-to-string SEXP so it's probably smarter to let them do that than to complicate things with another SEXP. We only really need the string version.
Ok, good, so FRED supports type casting. That makes the list-has-data/map-has-key/map-has-data-item/list-data-index SEXPs easier to code, since the only type the SEXPs need to support is strings.


By the way, the Edit Containers dialog won't let me create a container with a space in it but I'm pretty sure the code does support that. I've certainly had no issues so far with the containers I created before that restriction was added. Of course, if it makes things easier to limit containers in this way, I have no objection.
As long as FSO/FRED can handle containers with spaces in their names without a problem, there's no reason to have that restriction, so I'll remove it.

In other news, I think I figured out the bug you reported about infinite loops in container reference text replacement. :) I'll test it in the morning.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 06, 2021, 12:54:42 am
Builds in OP updated. AFAICT the issues you reported are fixed.

Spaces are now allowed in container names.

The SEXPs we discussed have been updated.

I also fixed an assertion failure in the edit containers dialog that occurred when you press Remove while nothing is selected.

With map-has-data-item, if the optional arg for the variable to hold the key is provided, we could check the type of the map key and compare it to the type of the variable, but I haven't done that type checking yet. Updating the variable with the key should work, though, but I haven't tested it.

I've also rebased against latest master.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 06, 2021, 09:19:32 pm
Cool. I'll test that now.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 09, 2021, 11:04:43 am
Next question: What should be the text length limits for container names, map keys, and container data?

The theoretical limit is 65 (2 * TOKEN_LENGTH + 1)  which is the largest string that will fit in sexp_list_item::text (including null char) and is already the max length allowed for variable names.

However, to ensure that multidimensionality is possible, the max data length must be at least 2 chars longer than the max container name length.

The simplest limits:

- 29 chars for container name (TOKEN_LENGTH -1 (null char) - 2 (leave space for enclosing &))
- 31 chars for map key and map/list data (TOKEN_LENGTH - 1)

I can find some ways to enforce these limits, e.g., through the text edit boxes in FRED's edit containers dialog.

Thoughts?
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 09, 2021, 10:10:26 pm
The only issue I'm worried about is if we end up with a situation where the data or key length is shorter than other arguments that people will try to use it for. For instance, if you name your map keys after ships and then have a ship with a 32 character name, it can't ever match the key.

I'm fine with whichever is easier to enforce.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 10, 2021, 12:40:11 am
AFAICT 31 is the max length for ship names. A limit of 31 chars makes sense, because NAME_LENGTH is 32 chars and you need space for the terminating null char. :)

You can see the limit enforced in this line in CShipEditorDlg::DoDataExchange() in the FRED code:
Code: [Select]
DDV_MaxChars(pDX, m_ship_name, NAME_LENGTH - 1);

Sure enough, when I opened the ships editor in FRED and typed a ship name that was too long, I get a pop-up with "Enter no more than 31 characters."
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 10, 2021, 01:59:03 am
Yeah. That does seem to make sense then.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 10, 2021, 11:59:20 pm
Looks like the issue I said might be on my to-do list still is there, I just didn't remember it correctly. If you attempt to edit a container in a mission briefing nothing happens. I'm attaching a mission that has 4 pilots in the Traitors map. The first briefing stage has this SEXP in it.

perform-actions
-true
-remove-from-map
--Traitors
--Capricorn

That should remove Capricorn from the list. Yet when the briefing message is displayed the string &Traitors&Capricorn&Callsign& is still replaced with the callsign for Capricorn from the map with that name.

Briefing stage 2 and 3 are a test whether using the &BriefingPilotList&Remove_Random& syntax works and it does work correctly. Stage 2 should give you the names of the pilots except for Capricorn. Stage 3 should be nonsense since the list should now be empty. I suspect that removing Container_edits_off as we discussed might solve the problem so you might as well do that before running any tests.

[attachment deleted to save space]
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 11, 2021, 05:01:45 pm
Turns out the issue isn't exactly a "bug" but more of a design choice.

Container text replacement for briefings was happening right after variable replacement, which is long before any briefing SEXPs are evaluated.

I moved container text replacement to take place for a given briefing stage after the stage's SEXPs are evaluated, and now the behavior is what you expected.

Note that there is a warning for Stage 1, because Capricorn is not in the Traitors map. The later failed replacements don't result in warnings because BriefingPilotList is empty at that point. We can add warnings for text replacement failures with empty maps/lists, if you'd like.

BTW, after replacement,  Stage 2 wingman names are Zodiac signs, since those are the keys of Traitors. :)

While I'm working on text replacement, are there any other cases where container replacement should happen at some point other than right after variable replacement? Debriefings, maybe, and presumably only if the debriefing stage is actually added?

I'll update the builds once these questions are sorted out.


EDIT: Also, could a FREDder reasonably want to use '&' in player-facing text? IF so, should we pick a different character to be the container delimiter? Maybe '@'? It would be ideal to avoid spurious warnings when running debug builds just because the FREDder wanted to use an '&' in in-game text.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 11, 2021, 11:48:51 pm
Yeah. I could see them using an "&" in-game now that I think about it. Wouldn't @ also be likely to cause issues? If any player thinks we're still using email in 200 years time they might have used that character too.


Turns out the issue isn't exactly a "bug" but more of a design choice.

Container text replacement for briefings was happening right after variable replacement, which is long before any briefing SEXPs are evaluated.

I moved container text replacement to take place for a given briefing stage after the stage's SEXPs are evaluated, and now the behavior is what you expected.

I did consider that as a possibility though. I originally added stages 2 & 3 to test for that. Stage 3 was originally just a repeat of Stage 2 without the perform actions. Unless ALL the briefing stages are created and only then do they evaluate to see if they are played or not stage 3 should have worked properly but it didn't (at least when I tried it). I'll test the behaviour once I have the builds and see what happens.

This does raise a question of whether the replacement for SEXP variables is also in the wrong place. If a briefing contains "We now still face $MyVariable$ Sathanases" and the briefing SEXP is

perform-actions
-previous-event-true
--Sathanas-Destroyed
-modify-variable
--MyVariable[10]
--- -
---MyVariable[10]
--- 1

I would expect the text displayed to change from "We now still face 10 Sathanases" to We now still face 9 Sathanases" if one was destroyed last mission. But from what you've said it sounds like we'd get the former message and only afterwards would the perform-actions be evaluated. Whatever we do with containers should be consistent with variables whenever possible. So I think it would be a good idea to get some other coders (especially Goober) to weigh in on this one.

Quote
Note that there is a warning for Stage 1, because Capricorn is not in the Traitors map. The later failed replacements don't result in warnings because BriefingPilotList is empty at that point. We can add warnings for text replacement failures with empty maps/lists, if you'd like.

Stage 3 (and 2 I think) should definitely result in warnings because of the attempts to use an empty array. As discussed yesterday, FREDders should run their missions through fast debug builds to catch errors like those. End Users don't need to worry about this sort of error so a warning is appropriate.

Quote
BTW, after replacement,  Stage 2 wingman names are Zodiac signs, since those are the keys of Traitors. :)

Yeah. I'd originally written that stage using the Traitors map and forgot I changed to a list.

Quote
While I'm working on text replacement, are there any other cases where container replacement should happen at some point other than right after variable replacement? Debriefings, maybe, and presumably only if the debriefing stage is actually added?

Pretty much anywhere SEXPs can be used in text should have replacement occur after evaluation. Debriefs, command briefs, fiction viewer, etc. I'm not sure if in those cases the replacement of variables happens then. So we might need to check those for the same possible issue briefings have.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: Goober5000 on April 12, 2021, 12:06:49 am
This is an interesting situation.  You're pushing the entire briefing system (text and SEXPs taken holistically) quite a bit farther than it was originally intended to go.  That's not a bad thing, though.

It seems reasonable that SEXP evaluation should take place before text replacement, as karajorma said.  Whether that means moving the SEXP evaluation earlier or the text replacement later might require deeper investigation.  My first reaction is to move the SEXP evaluation as early as possible because one could argue that the entire briefing context depends on it.  For example, future coders might add the ability to swap out the animation.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 12, 2021, 12:51:07 am
Yeah. I could see them using an "&" in-game now that I think about it. Wouldn't @ also be likely to cause issues? If any player thinks we're still using email in 200 years time they might have used that character too.
True, and there are other less frequently used characters that might work, such as ` ^ and |. My vote among those is ^ because it's the easiest to get to on the keyboard and is near $ which FREDders are already used to using as a metacharacter.

This is an interesting situation.  You're pushing the entire briefing system (text and SEXPs taken holistically) quite a bit farther than it was originally intended to go.  That's not a bad thing, though.
Couldn't you say the same thing about practically every part of the engine/editor? :)

It seems reasonable that SEXP evaluation should take place before text replacement, as karajorma said.  Whether that means moving the SEXP evaluation earlier or the text replacement later might require deeper investigation.  My first reaction is to move the SEXP evaluation as early as possible because one could argue that the entire briefing context depends on it.  For example, future coders might add the ability to swap out the animation.
Yup, makes sense. It'll require a case-by-case investigation of the code for briefings, debriefings, fiction viewer, etc., though, to figure out where text replacement and SEXP evaluation should go. That sounds like a project of its own. I can look into starting a separate branch/PR to explore that for variables. I'll keep this task focused on containers.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 12, 2021, 12:54:50 am
Yep. I agree on the separate branch.

It seems reasonable that SEXP evaluation should take place before text replacement, as karajorma said.  Whether that means moving the SEXP evaluation earlier or the text replacement later might require deeper investigation.  My first reaction is to move the SEXP evaluation as early as possible because one could argue that the entire briefing context depends on it.  For example, future coders might add the ability to swap out the animation.

I'd tend to agree with that. A FREDder would probably assume that the first thing that is done is that the SEXP is evaluated to see if that briefing stage even plays in the first place and only after that is anything else for that stage dealt with. And that does seem like a perfectly reasonable assumption to make.

Hell, you could even make the argument that if that stage doesn't play, the game should move on to the next one. In cases where the briefing isn't being played, you definitely shouldn't be doing text replacement with containers since that can alter the contents of a container for a briefing stage the mission designer would expect to not be touched. So it might be an idea to deal with this issue first (in a separate branch of course) and then deal with containers.

But feel free to give me some new builds so I can make test missions for this.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 12, 2021, 01:13:01 am
Ok, builds updated with the fix. Var replacement is unchanged.

The changes from the last build began with commits on April 8: https://github.com/jg18/fs2open.github.com/commits/feature/sexp-containers-revised
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 14, 2021, 07:27:55 am
Here's a quick test mission I put together to show what values in the briefings and debriefs would be expected by a FREDder. As you can see they're not correct.

[attachment deleted to save space]
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 14, 2021, 10:15:51 pm
I created a PR that should fix this (https://github.com/scp-fs2open/fs2open.github.com/pull/3385).

EDIT: removed link to test build (no longer needed since the fix is now in master).
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 21, 2021, 11:39:30 pm
Builds in the OP updated with minor fixes and cleanup.

Hoping to start a PR in the next few days.

The remaining TODOs can get sorted out as part of PR review.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 26, 2021, 01:37:42 am
Builds in the OP updated with (untested) container persistence support.

The only missing part is FRED controls to enable it. However, you can add the persistence options in Notepad, which should go after the $Data line.

The options (case-insensitive, include the '+'):

+Eternal
+Save On Mission Progress
+Save On Mission Close

"+Eternal", if used, must come before the "Save On" option.

Even though FRED doesn't have controls yet, it should still load/save the container persistence options.

EDIT: Forgot to mention that persistent containers should work for "Red Alert" missions as well.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 26, 2021, 03:51:50 am
Awesome. Will download and test now.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: karajorma on April 29, 2021, 02:13:14 am
A couple of minor issues I just noticed while putting together a campaign to test persistence.

Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on April 30, 2021, 04:42:48 pm
Builds in OP updated with FRED controls for persistence options and also a formatting fix when saving persistence options to disk.

For the UI glitches with container renaming, I'm thinking of using a separate dialog to rename containers, something similar to the Add New Container dialog, and then making the list of container names non-editable. That'll make it much easier to fix those bugs.
Title: Re: Continuing Karajorma's work on SEXP containers
Post by: jg18 on May 03, 2021, 12:09:34 am
Builds in OP updated.

The main change is that the drop down box of container names is no longer editable. Instead, there's a new "Rename Container" button.

The container names box is now disabled when the last container is deleted.

The FRED UI glitch about a renamed container's name not getting updated in the drop down box should now be fixed.

I also rebased against latest master, which now includes my fix for variable replacement.