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

Lua no global sharing #3780

Draft
wants to merge 5 commits into
base: MultiScriptDrawing
Choose a base branch
from
Draft

Lua no global sharing #3780

wants to merge 5 commits into from

Conversation

SuuperW
Copy link
Contributor

@SuuperW SuuperW commented Sep 25, 2023

This PR builds on another PR (#3775). (So don't merge this until/unless that one is merged/approved.)

This PR fixes issue #3036. I'm not certain if this is actual desired behavior, since that issue was marked as wontfix but a stated reason for that was "fixing this within the current system would suck". So it's unclear if a fix is wanted, or wasn't done for other reasons.

@YoshiRulz YoshiRulz changed the base branch from master to MultiScriptDrawing September 25, 2023 19:50
@YoshiRulz
Copy link
Member

The fact that globals are shared isn't great, but we'd want to have a replacement merged and field-tested before disabling this.

@SuuperW
Copy link
Contributor Author

SuuperW commented Sep 26, 2023

I did a force-push. The only change compared to previous commmit is in NamedLuaFunction.DetachFromScript.

The fact that globals are shared isn't great, but we'd want to have a replacement merged and field-tested before disabling this.

Do you mean that sharing globals should be an option available to the user? so that we can also keep the old behavior until this has been field-tested

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented Sep 26, 2023

I don't like this. Especially with running simple commands in the Lua Console, as now it can no longer access global variables in scripts (something I've used before as a way to toggle options on/off or just call manually call functions within the script). Scripts sharing globals as is is just a feature. At most we could just have an option to reset Lua state (without the user needing to close/re-open the console), and perhaps a way to open a script with a new lua state (but this not being the default). This PR as-is I will revert if merged.

@SuuperW
Copy link
Contributor Author

SuuperW commented Sep 26, 2023

I don't like this. Especially with running simple commands in the Lua Console, as now it can no longer access global variables in scripts (something I've used before as a way to toggle options on/off or just call manually call functions within the script).

I pushed a new commit which may suit your use case. You can now toggle options again if the script is currently highlighted. Please let me know if this is a good solution.

At most we could just have an option to reset Lua state (without the user needing to close/re-open the console)

I don't see how this would solve any issues. Two incompatible scripts would still be incompatible.

perhaps a way to open a script with a new lua state (but this not being the default).

I think making it a setting for the user to choose would make sense. Defaulting to preserve old behavior also makes sense.

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented Sep 26, 2023

I don't see how this would solve any issues.

One of the issue of everything sharing globals means if a script has trashed globals you can't restore it by loading up a new script nor reloading the offending script (probably after the user modifies it), you need to close the lua console and re-open it.

I pushed a new commit which may suit your use case. You can now toggle options again if the script is currently highlighted. Please let me know if this is a good solution.

This isn't a good idea, this behavior wouldn't be obvious to any user, and as you've made it it wouldn't work at all if multiple scripts are highlighted? (or it does for only 1 of them? I can't tell from the code).

@vadosnaprimer
Copy link
Contributor

If it shouldn't be merged yet, you can convert it to a draft.

@SuuperW SuuperW marked this pull request as draft September 28, 2023 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants