Skip to content
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

Externalize script files #356

Closed
wants to merge 7 commits into from

Conversation

mwpenny
Copy link
Contributor

@mwpenny mwpenny commented Jul 8, 2020

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

mwpenny added 2 commits July 7, 2020 20:28
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.
#include "scripts/generated/gamecomplete_ending.vsc.cpp"
#include "scripts/generated/startepilogue.vsc.cpp"
#include "scripts/generated/returntolab.vsc.cpp"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@InfoTeddy
Copy link
Contributor

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
Copy link
Contributor Author

mwpenny commented Jul 8, 2020

@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
Copy link
Contributor

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

ENDIF()

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@InfoTeddy InfoTeddy Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Contributor Author

mwpenny commented Jul 8, 2020

@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
Copy link
Contributor Author

mwpenny commented Jul 8, 2020

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

@mwpenny
Copy link
Contributor Author

mwpenny commented Jul 8, 2020

@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
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Contributor Author

mwpenny commented Jul 8, 2020

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
Copy link
Owner

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
Copy link
Collaborator

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
Copy link
Contributor Author

mwpenny commented Jul 8, 2020

@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
Copy link
Contributor Author

mwpenny commented Jul 13, 2020

@flibitijibibo is there anything preventing this from being merged?

@flibitijibibo
Copy link
Collaborator

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
Copy link
Contributor Author

mwpenny commented Jul 13, 2020

@flibitijibibo ah okay, sounds good!

@InfoTeddy
Copy link
Contributor

@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
Copy link
Contributor Author

mwpenny commented Jul 25, 2020

@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
Copy link
Contributor

Daaaav commented Jul 25, 2020

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

mwpenny commented Sep 14, 2021

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
Copy link
Contributor Author

mwpenny commented Sep 14, 2021

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
Copy link
Contributor

Daaaav commented Sep 18, 2021

@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
Copy link
Contributor

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

@mwpenny
Copy link
Contributor Author

mwpenny commented Jul 7, 2024

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 mwpenny closed this Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants