In the latest nightly build regions are broken

[oskarkk]

Skript/Server Version

Tested on clean Paper 1.18.2 with WorldGuard/Edit and Skript nightly build from https://github.com/SkriptLang/Skript/actions/runs/1952859162

Bug Description

Something must have changed regarding at least region contains expression, it just isn't parsed correctly anymore.

Expected Behavior

The same as in the latest stable version.

Steps to Reproduce

1. Use the latest nightly Skript build.
2. Use this script:

command /test:
  trigger:
    if player is in region "testregion":
      message "testtest"

3. /sk reload scripts It won't parse.

Errors or Screenshots

[04:54:21 Server thread/ERROR] [Skript] Line 3: (test.sk)
    Can't compare a player with 'in region "testregion"'
    Line: if player is in region "testregion":

Other

I also tried it on a nightly build made 2 months ago (https://github.com/SkriptLang/Skript/actions/runs/1757620182) and it was working.

Agreement

• I have read the guidelines above and affirm I am following them with this report.

[AyhamAl-Ali]

Nightly build starting from this PR caused the issue

• Fix VariableString converting #4408

[TPGamesNL]

Should be fixed by adding String -> Region converter

[Pikachu920]

Should be fixed by adding String -> Region converter

i'm not sure if the situation has changed, but a couple years ago registering a string -> something converter caused a bunch of bizarre issues. we would probably want to test this carefully

[oskarkk]

I'm not sure if I understand how it works, but here's my thoughts. When you're talking about converting string to region, that means that you must have to interpret code like this:

if player is in "regionname":
  message "player in region"

Where the syntax is:

[[the] region] %regions% contain[s] %directions% %locations%
%locations% (is|are) ([contained] in|part of) [[the] region] %regions%
[[the] region] %regions% (do|does)(n't| not) contain %directions% %locations%
%locations% (is|are)(n't| not) (contained in|part of) [[the] region] %regions%

So, the two variables in that expression are player and "regionname". The second is a string, so you must convert it to region, and that's were it fails in these nightly builds, am I right?

I want to say that I think this is bad design. Condition like player is in "regionname" shouldn't be allowed, it's unclear, it should be more explicit. Only syntaxes like player is in region "regionname" should be allowed. And [the] region "regionname" should be an expression, so you could do:

set {_region} to region "regionname"
if player is in {_region}:
  message "player in region"

But, it would still be a broken design. Overall I think most of region handling in Skript is fundamentally broken, and here's one of the reasons: in WorldGuard, region names are unique only for worlds, not for servers. So you can have region with the same name in both overworld and nether, and they'd be separate entities. Look at this code:

	Region r = null;
	for (final World w : Bukkit.getWorlds()) {
		@SuppressWarnings("null")
		final Region r2 = RegionsPlugin.getRegion(w, s);
		if (r2 == null)
			continue;
		if (r != null) {
			Skript.error("Multiple regions with the name '" + s + "' exist");
			return null;
		}
		r = r2;
	}
	if (r == null) {
		Skript.error("Region '" + s + "' could not be found");
		return null;
	}
	return r;

Imagine you have region "test" in overworld and "test" in nether. With code player is in "test" you will always get the overworld region (I think), and there's no method to get that nether region. You'd have to rename your regions to be compatible with Skript.

So, the region-getting expression should be region %string% in %world%. Or maybe world could be optional and would default to the event-world, if this can be done.

There's more I could say about regions, but that would be another issue. I think that instead of fixing this issue directly, it should be fixed by changing the bad design and syntax.

[AyhamAl-Ali]

Should be fixed by adding String -> Region converter

i'm not sure if the situation has changed, but a couple years ago registering a string -> something converter caused a bunch of bizarre issues. we would probably want to test this carefully

Testing is important indeed though we've done this before and haven't found any issue yet, the parser had many improvements over the past time so it might have fixed such issue

[TPGamesNL]

RE: @oskarkk (because I'm not quoting that whole message)

Adding the converter will not change anything about the syntax (region literals already have the same syntax as strings: "<regionname>). If you think the syntax is bad, it should go in another issue. However, I doubt much will be changed now that we plan to move the region stuff out of Skript, into an official addon.

[oskarkk]

I know that adding a converter wouldn't change the syntax. I proposed a new expression for region, because that would be very useful, and because then, if I understand correctly, the converter wouldn't be needed.

[TPGamesNL]

Indeed, if we add such an expression and we remove the literal for regions, the converter would no longer be needed. However, it should still go into a new issue.