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

Make EffContinue iterative instead of recursive #4111

Merged
merged 2 commits into from
Jun 25, 2021

Conversation

TPGamesNL
Copy link
Member

@TPGamesNL TPGamesNL commented Jun 25, 2021

Description

Makes EffContinue iterative, instead of recursive.

Test code:

command /test:
	trigger:
		loop 10000 times:
			continue

Target Minecraft Versions: any
Requirements: none
Related Issues: #4096

@TPGamesNL TPGamesNL added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Jun 25, 2021
@Mwexim
Copy link
Contributor

Mwexim commented Jun 25, 2021

We had a similar issue at skript-parser, and I spend hours trying to find the core problem.

Make sure that all sections that run code multiple times (loops and while-sections for example) are self referencing. Not sure how it is implemented here exactly, but at skript-parser, SecLoop was recursive as well and this lead to huge issues.

@TPGamesNL
Copy link
Member Author

We already do that in Skript

Copy link
Contributor

@ShaneBeee ShaneBeee left a comment

Choose a reason for hiding this comment

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

tested, runs beautifully 👍

@ShaneBeee ShaneBeee merged commit c7eb11c into SkriptLang:master Jun 25, 2021
@TPGamesNL TPGamesNL deleted the fix/eff-continue-non-recursive branch June 25, 2021 15:04
@Pikachu920
Copy link
Member

@TPGamesNL thinking about this more: maybe it would be more appropriate to set the next item of the continue effect to the loop instead of overriding walk at all?

@TPGamesNL
Copy link
Member Author

@TPGamesNL thinking about this more: maybe it would be more appropriate to set the next item of the continue effect to the loop instead of overriding walk at all?

That would also work, but the only problem is that you can't set the next item in init, because there is no next item yet (the next line isn't parsed yet), so we'd have to do it some time later, which would make it weirder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants