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

Implement "resume reload" command. #2609

Merged
merged 2 commits into from
Jun 2, 2024
Merged

Conversation

technomancy
Copy link
Contributor

@technomancy technomancy commented Jun 1, 2024

When resuming, you can pass "reload" as the argument, which causes the cart's code to be reloaded before resuming the game.

This allows for a much smoother iterative development style where you can pause the game, make changes, and see the effect of those changes without restarting the game. It does require some special handling in the cart to support saving state to a global and avoiding that state being reinitialized if it's already set; for instance:

state = state or {x=25, y=100, shots={}}

This implements the reload method for the Lua and Fennel scripts, but it will need to be implemented for other scripts independently.

@technomancy
Copy link
Contributor Author

Sorry; I was looking back thru some of my other changes, and I realized that perhaps this can be implemented better in a way that doesn't require a new reload method to be added for all the existing scripts:

https://github.com/nesbox/TIC-80/pull/840/files

It's getting late here tonight, but tomorrow I will update my patch to do this instead; the way I have it currently is probably more complicated than it needs to be.

I thought that making reload an argument to the resume command would be a nice way to introduce this functionality without adding a whole new top-level command. However, we could add it as a top-level command, or we could make it so you run eval reload to do this instead. I'm open to suggestions if you think there is a better way.

When resuming, you can pass "reload" as the argument, which causes the
cart's code to be reloaded before resuming the game.

This allows for a much smoother iterative development style where you
can pause the game, make changes, and see the effect of those changes
without restarting the game. It does require some special handling in
the cart to support saving state to a global and avoiding that state
being reinitialized if it's already set; for instance:

    state = state or {x=25, y=100, shots={}}

This implements it in a way that will be supported by any script that
already supports eval.
@technomancy
Copy link
Contributor Author

OK, I've replaced the implementation with a much simpler one that works across every eval-supporting script, plus fixed a bug where eval for Fennel wasn't checking to see if the VM had been initialized yet. I've tested it, and it should be ready for review.

I think I will probably follow this up with a patch to make it so that Fennel will allow you to eval even if you haven't run yet by automatically initializing the Lua VM and loading the game code first instead of silently doing nothing. This seems like a lot more useful behavior.

@joshgoebel
Copy link
Collaborator

Also have you tested this with more dynamic runtimes and objects? Just because you redefine a class doesn't necessarily mean all the existing objects already instantiated will have that new API (or that they will have magically lost the old API). Ruby being one example where monkey patching is "additive".

This can lead to weird edge cases and bugs. I wouldn't recommend this be merged as-is.

@technomancy
Copy link
Contributor Author

Also have you tested this with more dynamic runtimes and objects?

I can imagine this causing problems on less dynamic runtimes; for instance if you have a language that refuses to allow you to redefine a class if the class is already defined.

But this is a purely additive patch. Nothing will change unless users specifically go out of their way to request a reload. If you work in a language that doesn't allow reloads, you are unlikely to run the resume command with the argument of reload, and if you do, the language's compiler should give an error that explains why what you tried to do won't work.

Ruby being one example where monkey patching is "additive".

It's been a few decades since I used Ruby, so maybe you can refresh my memory here about what could go wrong, but yes, the point of this patch is to allow additive changes to be made. It would certainly be very bad if it were triggered every time you ran resume, but that is not how it works.

In any case, developing games without this kind of functionality in Lua/Fennel is rather tedious, so I want to make sure it is exposed somehow. If you think moving it to an argument of the eval command instead (eval reload) would be clearer and/or safer, that should be an easy change to make, but I'd like to hear what @nesbox thinks.

If necessary, it could also be implemented specifically in evalFennel and evalLua rather than the eval console command, but then Moonscript/JS/Wren/etc would miss out on it. (I don't know enough about those languages to say if they have the same problems Ruby has.)

@joshgoebel
Copy link
Collaborator

It's been a few decades since I used Ruby, so maybe you can refresh my memory here about what could go wrong,

I may be thinking of Rails reloading where it first un-defines the classes, which is different than what you're doing here. So you could end up with two Enemys with entirely different APIs, etc...

if you do, the language's compiler should give an error that explains why what you tried to do won't work.

That sounds like a reasonable safety-rail.

but I'd like to hear what @nesbox thinks.

He seems open to many things, so wouldn't surprise me if he's into it.

@nesbox nesbox self-requested a review June 2, 2024 07:53
Copy link
Owner

@nesbox nesbox left a comment

Choose a reason for hiding this comment

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

I don't see any problems with this PR, especially since it is a separate parameter and doesn't affect the normal work of the resume command.
Thank you.

@nesbox nesbox added the console Issues related to console inside TIC-80 label Jun 2, 2024
@nesbox nesbox merged commit 921d5e6 into nesbox:main Jun 2, 2024
12 checks passed
@technomancy technomancy deleted the resume-reload branch June 2, 2024 16:32
@technomancy
Copy link
Contributor Author

technomancy commented Jun 2, 2024

Thanks!

How would you like me to handle documentation for this? It should be mentioned on https://github.com/nesbox/TIC-80/wiki/Console but if we add it now then people will expect it to be supported on the latest release. Should I add it with a note about (1.3+ only)?

Also it appears I forgot to update the help resume text so I will submit a separate patch for that.

@Skeptim
Copy link
Contributor

Skeptim commented Jun 2, 2024

Thanks for this commit, that should help a lot!

You can add it to the wiki mentioning it has been added in version 1.2

I think with this reload argument that would be a good idea to create a short wiki page (appearing in the sidebar) for the resume command which is going to be more useful now and also slightly more complex to use, I guess.
Typically I don't fully understand this part:

It does require some special handling in the cart to support saving state to a global and avoiding that state being reinitialized if it's already set; for instance:

state = state or {x=25, y=100, shots={}}

An example of a situation where this is necessary would help make this more accessible.
If you don't want to create a resume wiki page I can do it if you explain me this part.
(Maybe that's obvious, I am not the most competent one here but I try to make the wiki accessible to people who learn programming.)

Edit: I understand how we would need to do new_var = new_var or 42 if we just created new_var and therefore it needs to be initialized within TIC(). But I don't understand how state could be undefined if it is a table with all the variables we use.

@technomancy
Copy link
Contributor Author

I've added an explanation to the wiki here: https://github.com/nesbox/TIC-80/wiki/Reload

Let me know if it's clear or if anything could be improved.

@Skeptim
Copy link
Contributor

Skeptim commented Jun 8, 2024

That's clear, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues related to console inside TIC-80
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants