Externalize script files #356

mwpenny · 2020-07-08T00:30:19Z

Changes:

I would like to help clean up this codebase and make it more portable. The first step of that is organizational changes like this one.

Rather than existing in code, all scripts now exist in scripts/name.vsc. VSC stands for Vvvvvv SCript (I'm open to changing this). Before building, a new CMake script uses these .vsc files to generate .vsc.cpp files by wrapping the scripts in:

if (SDL_strcmp(t, "script_name") == 0) {
    static const char* lines[] = {
        // .vsc file contents with comments removed. E.g.,
	"line1",
	"line2",
	// ...
	"lineN",
    };
    filllines(lines);
    return;
}

Scripts.cpp is then as simple as a bunch of #includes for the generated code (I removed TerminalScripts.cpp and put everything in Scripts.cpp now that it is so small). I plan to make the script loading better in a future change (for example, by using a map to lookup the script to parse, rather than many ifs, but I think this is a good first step).

Let me know if you are not interested in these kinds of changes.

Legal Stuff:

By submitting this pull request, I confirm that...

My changes may be used in a future commercial release of VVVVVV (for
example, a 2.3 update on Steam for Windows/macOS/Linux)
I will be credited in a CONTRIBUTORS file and the "GitHub Friends"
section of the credits for all of said releases, but will NOT be compensated
for these changes

Rather than existing in code, all scripts now exist at scripts/name.vsc. VSC stands for Vvvvvv SCript (I'm open to changing this). Before building, a new CMake script uses these .vsc files to generate .vsc.cpp files by wrapping the scripts in: ```c++ if (SDL_strcmp(t, "script_name") == 0) { static const char* lines[] = { // .vsc file contents with comments removed. E.g., "line1", "line2", // ... "linen", }; filllines(lines); return; } ``` Scripts.cpp is then as simple as a bunch of `#include`s for the generated code.

InfoTeddy · 2020-07-08T00:35:25Z

desktop_version/src/Scripts.cpp

+    #include "scripts/generated/gamecomplete_ending.vsc.cpp"
+    #include "scripts/generated/startepilogue.vsc.cpp"
+    #include "scripts/generated/returntolab.vsc.cpp"
+


There is trailing whitespace on this line (line 180).

InfoTeddy · 2020-07-08T00:38:07Z

I plan to make the script loading better in a future change (for example, by using a map to lookup the script to parse, rather than many ifs

As long as it isn't an STL map, this should be fine.

mwpenny · 2020-07-08T00:38:58Z

@InfoTeddy this is all conceptual at this point. I haven't thought about concrete implementations yet. However, why avoid an STL map? Are you worried about memory fragmentation?

InfoTeddy · 2020-07-08T00:42:51Z

@flibitijibibo can answer why we want to avoid STL, but for me personally I just like smaller binary size.

InfoTeddy · 2020-07-08T00:46:25Z

desktop_version/cmake/GenerateScriptCode.cmake

+		ENDIF()
+
+		# Remove inline comments
+		STRING(REGEX REPLACE "#.+$" "" LINE "${LINE}")


It's nice that you specifically added code to deal with comments (and want to preserve them), but the scripting engine simply ignores all invalid commands, including comments. So I'm not really sure how necessary this is.

That's good to know. One additional benefit of this is that the size of the static string arrays will be (marginally) smaller. In any case, leaving the comments out of the generated code is cleaner to me but I could go either way.

Does the scripting engine ignore invalid text at the end of an otherwise valid line?

Yes, as long as you always close the arguments to the command with a parenthesis at the end.

That's 'cause it splits lines into arguments, using an argument separator token, which can be any one of (, ,, or ) (normal language rules like having to match parentheses don't apply, delay,5( is a valid script line). If you go with the "proper" style, usually you'll have a parenthesis at the end closing off the arguments, e.g. delay(5). And if you have a comment, then it's fine, because delay(5) # comment gets parsed as delay, 5, and #comment (the engine also strips out spaces as well), and delay() ignores the second argument so it's all good. But if you have delay(5 # comment, it gets parsed as delay and 5#comment, which isn't what you want (but funnily enough it's fine enough because number parsing in scripts uses ss_toi(), which takes in as many digit characters as it can until it doesn't find any more, so it correctly identities that you want to delay for 5 frames).

Hence my recommendation above.

flibitijibibo · 2020-07-08T01:07:59Z

This might be something where @TerryCavanagh should make the call here, but in my head this is a good idea that sidesteps the main issue with the scripts, which is that they're built into the source (neatly or otherwise).

Normally I like having the assets compiled in, but in this case the game logic got kinda coupled together with the scripts, so effectively the paid portion of the game was in the source code (which was one of the big motivators for the license that ended up getting used). On one hand this at least separates the two, but if we're getting to the point where the scripts are fully separate files, it may be a good time to evaluate whether or not we want to move the scripts out of the source entirely and into a separate data archive that only ships with the paid game. It'd get a lot of the "still-proprietary-but-not-really" stuff out of the repo (not the commit log, but oh well...) and would make data.zip a little less scary to throw around, since the real part of the main game would be somewhere else.

This is all just a not-at-all thought out stream of consciousness though, in this case I'd say it's not my decision to make.

TerryCavanagh · 2020-07-08T01:13:43Z

This looks good to me - am I misremembering or are there hardcoded scripts that use variables that can change at run time? Maybe that was in a different project. Well, if it doesn't break anything, then this change seems harmless to me.

As for externalising the scripts and loading them all at runtime instead of compiling them in: I'd be in to that! (Though I don't know how useful it would really be, seeing as custom levels already have a separate way to do scripting). Still, no objections here.

mwpenny · 2020-07-08T01:20:08Z

@flibitijibibo I like your idea of having a separate archive for the paid game. I think moving the scripts out "for real" (along with the levels) is a great goal to work towards and I think this change helps move things in that direction, as a sort of first step. In my opinion, game content should be treated as a runtime resource, and not code like it is now. It would make it easier for users to create custom campaigns too.

Ideally, we could get to a point where the source is just the engine - decoupled from the paid content - and nothing else. An added benefit of this is that it would almost certainly allow the code to become cleaner (as we can see a little here). And of course the legal/licensing points you brought up as well.

@TerryCavanagh, is that something you're open to?

mwpenny · 2020-07-08T01:21:39Z

@TerryCavanagh I was typing this while you replied. Sounds good!

mwpenny · 2020-07-08T01:22:32Z

@TerryCavanagh All of the scripts are strings which don't have any run time variables included. They only interface with the game via the scripting API

TerryCavanagh · 2020-07-08T01:24:33Z

For what it's worth, I think I'd like to avoid having to update data.zip and risk having separate versions floating around - i.e. a 2.2 data.zip and 2.3 data.zip sounds like a mess to me. Even if the in-game script files end up being moved to external files at runtime, they should just be included with both the commercial version and the make and play versions!

InfoTeddy · 2020-07-08T01:27:43Z

Yeah, I agree with Terry. Especially since I can name at least one custom level that directly calls a main game script!

Also I think that there ought to be a way to just bake these scripts in to the binary at compile time, without having to have CMake auto-generate source code files for us.

mwpenny · 2020-07-08T01:29:04Z

I was thinking there could be data.zip, which contains the base assets like it does now (i.e., common to the paid game and M&P), and game.zip or something with the scripts (and maybe levels). Then game.zip could be shipped with only the paid game.

This is all hypothetical at this point. Ultimately up to you

TerryCavanagh · 2020-07-08T01:34:34Z

Two zip files sounds ok to me, but I'd just ship the hypothetical game.zip file with all versions I think. As @InfoTeddy points out, people have done some weird stuff with custom VVVVVV levels and making sure nothing breaks is the most important thing here.

flibitijibibo · 2020-07-08T01:34:40Z

The "new" assets would definitely go in a separate game.zip - we can mount the second archive without modifying the original and it'll still all act as a single filesystem, so no trouble there!

mwpenny · 2020-07-08T01:34:44Z

@InfoTeddy good call on custom levels that use main game scripts. That would mean they should be shipped with both versions then.

Technically this is baking it into the binary :p Although instead of this, in a future change I can embed them as binary resources instead of doing the code generation. Then they could be looked up at runtime. That would be a lot cleaner (no need for the #includes). Just wanted to get this first step merged in before building upon it. If we're all okay with game.zip though, I think that's best since it keeps the scripts out of the compiled binary

mwpenny · 2020-07-13T05:31:04Z

@flibitijibibo is there anything preventing this from being merged?

flibitijibibo · 2020-07-13T15:27:09Z

The feature freeze is still in place, so this will have to wait until after 2.3 is tagged. I do still wonder about how separating the scripts should go, but we'll worry about that after the 2.3 bugs are all resolved.

mwpenny · 2020-07-13T17:52:20Z

@flibitijibibo ah okay, sounds good!

desktop_version/src/scripts/terminal_finallevel.vsc

…erryCavanagh#357

InfoTeddy · 2020-07-25T19:51:24Z

@mwpenny You should also rebase onto the latest commit of master (it contains fixes for some of Vitellary's facing changes during his dialogue in Intermission 1).

mwpenny · 2020-07-25T19:55:08Z

@Dav999-v I've fixed the scripts. There were other spacing errors too. Please let me know if you find other issues.

@InfoTeddy already done while I was fixing the other stuff :)

I prefer merge over rebase so the commit history is preserved. Then @flibitijibibo can squash and merge into master so it's only one commit, with my individual commit messages persisted in the commit message of the merge commit.

Daaaav · 2020-07-25T21:35:21Z

I wanted to see if I could find any more differences in the .vsc files, so I wrote some code that loads all the original non-externalized scripts and writes them to separate files. I was going to say I did find a few more places where whitespace at the start of lines was reduced and the fact that PR #357 wasn't applied, but I got distracted and it seems like I got ninja'd on both of these :P

Either way, these are the script files I generated (disadvantage is of course that these don't have all the blank lines and comments). I'll compare your updated scripts to them in a bit!

Edit: I don't think there are any further inconsistencies.

InfoTeddy · 2021-09-13T17:44:57Z

This wasn't tagged as 2.4, so I kinda missed it in my initial round of merging 2.4 PRs. Per Ethan's comment, this is now allowed to be merged in for 2.4.

The only things that this PR needs is (a) a rebase, and (b) to account for any changes to the scripts since then (the only thing that I think needs updating is the new disableaccessibility script; @Dav999-v, can you run your scripts again to check for inconsistencies?). It'd be nice if you could also provide the scripts you used to generate the separate script files, so others can independently verify the scripts are the same.

flibitijibibo · 2021-09-13T17:55:55Z

This change is pretty big and technically affects content too, so to buy us some time I'd like to push this back to 2.5. Localization will be a big enough change as it is, so let's reduce the scope sooner rather than later and make externalized scripts the star of 2.5 instead (along with the mobile backport stuff).

mwpenny · 2021-09-14T00:52:47Z

I've updated my branch with the latest changes from master. There were a few small script changes which I've incorporated. Here's the script I've been using to generate the .vsc files: https://gist.github.com/mwpenny/7b104d682bbb618a0532d1258964796b

E.g.,

> python extract_vvvvvv_scripts.py src/Scripts.cpp src/scripts
> python extract_vvvvvv_scripts.py src/TerminalScripts.cpp src/scripts

Of course, Scripts.cpp and TerminalScripts.cpp have to be from the master branch.

The version of CMake used by this project does not support it. Also switch to using lowercase command names.

mwpenny · 2021-09-14T01:05:26Z

The version of CMake specified in CMakeLists.txt (2.8.12) predates the continue command. The Windows and Mac build environments happen to have CMake versions new enough to support it anyway. The Linux environment doesn't, so I've refactored my script to not need it (although the CMake version should probably be updated - it's 10 years old).

Daaaav · 2021-09-18T16:58:32Z

@InfoTeddy I should check my email more often...

I think everything's consistent again. As for my extraction and comparison scripts... Here they are, it's a bit "just make it work" level and it needs changes to be made to copied versions of Scripts.cpp and TerminalScripts.cpp to remove the classes, plus it actually relies on scripts to already be extracted. But that's something to be expected for a temporary one-off thing :)

vvv_extract_scripts.zip

AllyTally · 2022-11-12T23:25:18Z

If #906 gets merged, scripts might need to be re-extracted.

mwpenny · 2024-07-07T22:16:30Z

This PR has been open for 4 years now so I'm going to close it.

Thanks for the discussion, and feel free to use it if you'd like. All the best!

mwpenny added 2 commits July 7, 2020 20:28

Return after loading custom script

7cb86be

InfoTeddy suggested changes Jul 8, 2020

View reviewed changes

Remove trailing whitespace

be924f6

InfoTeddy reviewed Jul 8, 2020

View reviewed changes

Daaaav reviewed Jul 25, 2020

View reviewed changes

desktop_version/src/scripts/terminal_finallevel.vsc Outdated Show resolved Hide resolved

Daaaav reviewed Jul 25, 2020

View reviewed changes

desktop_version/src/scripts/terminal_finallevel.vsc Outdated Show resolved Hide resolved

Fix spacing and erroneous commas in scripts. Also update with fix for T…

7ac4251

…erryCavanagh#357

Merge branch 'master' into refactor-scripts

0531933

InfoTeddy added the required for 2.4 label Sep 13, 2021

flibitijibibo added required for 2.5 and removed required for 2.4 labels Sep 13, 2021

Merge branch 'master' into refactor-scripts

7b25286

Remove continue() from GenerateScriptCode.cmake

7f6f8dc

The version of CMake used by this project does not support it. Also switch to using lowercase command names.

flibitijibibo mentioned this pull request Feb 23, 2022

Forwards compatibility and scripting discussion #868

Open

InfoTeddy force-pushed the master branch from 4b14d5c to 9a40993 Compare December 6, 2023 00:14

mwpenny closed this Jul 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Externalize script files #356

Externalize script files #356

mwpenny commented Jul 8, 2020

InfoTeddy Jul 8, 2020

InfoTeddy commented Jul 8, 2020

mwpenny commented Jul 8, 2020 •

edited

Loading

InfoTeddy commented Jul 8, 2020

InfoTeddy Jul 8, 2020

mwpenny Jul 8, 2020

mwpenny Jul 8, 2020

InfoTeddy Jul 8, 2020 •

edited

Loading

flibitijibibo commented Jul 8, 2020

TerryCavanagh commented Jul 8, 2020

mwpenny commented Jul 8, 2020 •

edited

Loading

mwpenny commented Jul 8, 2020

mwpenny commented Jul 8, 2020

TerryCavanagh commented Jul 8, 2020

InfoTeddy commented Jul 8, 2020

mwpenny commented Jul 8, 2020 •

edited

Loading

TerryCavanagh commented Jul 8, 2020

flibitijibibo commented Jul 8, 2020

mwpenny commented Jul 8, 2020 •

edited

Loading

mwpenny commented Jul 13, 2020

flibitijibibo commented Jul 13, 2020

mwpenny commented Jul 13, 2020

InfoTeddy commented Jul 25, 2020

mwpenny commented Jul 25, 2020

Daaaav commented Jul 25, 2020 •

edited

Loading

InfoTeddy commented Sep 13, 2021

flibitijibibo commented Sep 13, 2021

mwpenny commented Sep 14, 2021

mwpenny commented Sep 14, 2021

Daaaav commented Sep 18, 2021

AllyTally commented Nov 12, 2022

mwpenny commented Jul 7, 2024

Externalize script files #356

Externalize script files #356

Conversation

mwpenny commented Jul 8, 2020

Changes:

Legal Stuff:

InfoTeddy Jul 8, 2020

Choose a reason for hiding this comment

InfoTeddy commented Jul 8, 2020

mwpenny commented Jul 8, 2020 • edited Loading

InfoTeddy commented Jul 8, 2020

InfoTeddy Jul 8, 2020

Choose a reason for hiding this comment

mwpenny Jul 8, 2020

Choose a reason for hiding this comment

mwpenny Jul 8, 2020

Choose a reason for hiding this comment

InfoTeddy Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

flibitijibibo commented Jul 8, 2020

TerryCavanagh commented Jul 8, 2020

mwpenny commented Jul 8, 2020 • edited Loading

mwpenny commented Jul 8, 2020

mwpenny commented Jul 8, 2020

TerryCavanagh commented Jul 8, 2020

InfoTeddy commented Jul 8, 2020

mwpenny commented Jul 8, 2020 • edited Loading

TerryCavanagh commented Jul 8, 2020

flibitijibibo commented Jul 8, 2020

mwpenny commented Jul 8, 2020 • edited Loading

mwpenny commented Jul 13, 2020

flibitijibibo commented Jul 13, 2020

mwpenny commented Jul 13, 2020

InfoTeddy commented Jul 25, 2020

mwpenny commented Jul 25, 2020

Daaaav commented Jul 25, 2020 • edited Loading

InfoTeddy commented Sep 13, 2021

flibitijibibo commented Sep 13, 2021

mwpenny commented Sep 14, 2021

mwpenny commented Sep 14, 2021

Daaaav commented Sep 18, 2021

AllyTally commented Nov 12, 2022

mwpenny commented Jul 7, 2024

mwpenny commented Jul 8, 2020 •

edited

Loading

InfoTeddy Jul 8, 2020 •

edited

Loading

mwpenny commented Jul 8, 2020 •

edited

Loading

mwpenny commented Jul 8, 2020 •

edited

Loading

mwpenny commented Jul 8, 2020 •

edited

Loading

Daaaav commented Jul 25, 2020 •

edited

Loading