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

Concurrency updates #3817

Merged
merged 6 commits into from
May 10, 2021
Merged

Conversation

TPGamesNL
Copy link
Member

Description

A few changes to AsyncEffect:

  • Update javadoc with a notice to ScriptLoader.hasDelayBefore = Kleenean.TRUE (otherwise the parser will think the event isn't delayed while it is).
  • Make local variables available in the effect itself, not just after it.
  • Cleaned up the code a bit.

Also updated EffLoadServerIcon to not throw an exception when its argument evaluated to null, and to set the hasDelayBefore field appropriately.


Target Minecraft Versions: any
Requirements: none
Related Issues: #3808 #1817

@TPGamesNL TPGamesNL added 2.5 bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. labels Mar 13, 2021
@TPGamesNL
Copy link
Member Author

Just noticed

/**
* Not to be accessed outside of Bukkit's main thread!
*/
private final static Map<Event, VariablesMap> localVariables = new HashMap<>();

@TPGamesNL TPGamesNL marked this pull request as draft March 14, 2021 16:14
@TPGamesNL TPGamesNL changed the title Fix AsyncEffect Concurrency updates Mar 15, 2021
@TPGamesNL TPGamesNL marked this pull request as ready for review March 15, 2021 16:51
@TPGamesNL
Copy link
Member Author

TPGamesNL commented Mar 15, 2021

@FranKusmiruk Should I edit VariablesMap to use ConcurrentHashMap and ConcurrentSkipListMap instead of HashMap and TreeMap?

PS PR didn't build because of downtime at Maven repo used by PaperLib

@FranKusmiruk
Copy link
Member

@FranKusmiruk Should I edit VariablesMap to use ConcurrentHashMap and ConcurrentSkipListMap instead of HashMap and TreeMap?

I believe that's out of the scope of this PR and still up for debate. I'd rather leave it for another PR so the bug fixes on this one can be merged faster.

@FranKusmiruk FranKusmiruk merged commit 132a559 into SkriptLang:master May 10, 2021
@TPGamesNL TPGamesNL deleted the fix/async-effect-locals branch May 10, 2021 08:47
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.

2 participants