Add player visibility stuff #1302

Blueyescat · 2018-06-10T20:24:08Z

Target Minecraft versions: Any
Requirements: No
Related issues: #1291

Description:
Adds effects to hide/reveal players for players, condition to check if a player can see another player and expression to get hidden players of a player.

Also updated Skript's PaperSpigot to 1.12.2 from 1.12.1 because i used the new hide/showPlayer(Plugin, Player) method that comes with 1.12.2 if it exists.

Thanks to @Snow-Pyon for some help on the code.

hide %players% (from|for) %players%
reveal %players% (to|for|from) %players%
#no show effect because conflicts with open inv effect

[(all [[of] the]|the)] hidden players (of|for|from) %players%
[(all [[of] the]|the)] players hidden (from|for|by) %players%

%players% (is|are) [(1¦in)]visible for %players%
%players% can see %players%
%players% (is|are)(n't| not) [(1¦in)]visible for %players%
%players% can('t| not) see %players%

Snow-Pyon

Looks good overall.

Snow-Pyon · 2018-06-10T20:28:38Z

src/main/java/ch/njol/skript/conditions/CondCanSee.java

+	static {
+		Skript.registerCondition(CondCanSee.class,
+				"%players% (is|are) [(1¦in)]visible [for %players%]",
+				"[%players%] can see %players%",


I don't think the following:

if can see player:

makes sense, just make it mandatory.

And you can use a single parser mark here

Snow-Pyon · 2018-06-10T20:29:24Z

src/main/java/ch/njol/skript/conditions/CondCanSee.java

+import org.bukkit.Bukkit;
+
+@Name("Can See")
+@Description("Checks whether or not a player can see another player.")


or not is redundant. And I'd rather use the given players.

Snow-Pyon · 2018-06-10T20:30:48Z

src/main/java/ch/njol/skript/conditions/CondCanSee.java

+
+	@Override
+	public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, SkriptParser.ParseResult parseResult) {
+		setNegated(((matchedPattern == 2 || matchedPattern == 3) && parseResult.mark != 2) || parseResult.mark == 1);


just check if matchedPattern > 1 ^ parseResult.mark == 1 to negate it.

Snow-Pyon · 2018-06-10T20:47:32Z

src/main/java/ch/njol/skript/effects/EffPlayerVisibility.java

+	public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, SkriptParser.ParseResult parseResult) {
+		reveal = matchedPattern > 0;
+		players = (Expression<Player>) exprs[0];
+		if (reveal && players instanceof ExprHiddenPlayers) {


Skript's code style doesn't use braces on this kind of cases.

Which line?

I think you mean conditions, ok..

Snow-Pyon · 2018-06-10T20:48:49Z

src/main/java/ch/njol/skript/effects/EffPlayerVisibility.java

+import org.bukkit.event.Event;
+
+@Name("Player Visibility")
+@Description({"Change visibility of a player for the given players. If a player was hidden and relogs, it will be visible again.",


You said the same twice here.

… into visibility-stuff

Blueyescat · 2018-06-10T21:12:18Z

Wat, blame GitHub Desktop for the latest commit :O But at least it didn't break anything..

Pikachu920 · 2018-06-11T00:16:29Z

src/main/java/ch/njol/skript/effects/EffPlayerVisibility.java

+		reveal = matchedPattern > 0;
+		players = (Expression<Player>) exprs[0];
+		if (reveal && players instanceof ExprHiddenPlayers)
+			targetPlayers = (exprs.length > 1 && !exprs[1].isDefault()) ? (Expression<Player>) exprs[1] : ((ExprHiddenPlayers) players).getPlayers();


what is the deal with this exprhiddenplayers thing?

He wanted the following:

reveal hidden players for {_player}

to work this way:

reveal hidden players for {_player} to {_player}

And it makes sense imo.

@Blueyescat should comment it a bit then, the code is confusing

Already noted in the effect description.

no reason it can't be in both places

Pikachu920 · 2018-06-11T19:50:55Z

src/main/java/ch/njol/skript/effects/EffPlayerVisibility.java

+
+	static {
+		Skript.registerEffect(EffPlayerVisibility.class,
+				"hide %players% [(from|for) %players%]",


I really don't think event value assumption works well in these syntaxes, take this code:

on click: hide player

you would think that hides the player from everyone but in reality it hides the player from themselves which isn't a thing

Actually i can check if the expression is not the event player, but still confusing. I will remove defaulting to event player and i will make it defaulting to all players (this only valid for the effect).

Pikachu920 · 2018-06-11T19:54:35Z

src/main/java/ch/njol/skript/conditions/CondCanSee.java

+
+	static {
+		Skript.registerCondition(CondCanSee.class,
+				"%players% (is|are) [(1¦in)]visible [for %players%]",


these syntaxes have the same issue with event value assumption

Pikachu920 · 2018-06-11T19:55:12Z

src/main/java/ch/njol/skript/conditions/CondCanSee.java

+
+	static {
+		Skript.registerCondition(CondCanSee.class,
+				"%players% (is|are) [(1¦in)]visible [for %players%]",


why not make the visible/invisible separate syntaxes? like you said it's better for documentation

Pikachu920 · 2018-06-11T19:55:50Z

src/main/java/ch/njol/skript/effects/EffPlayerVisibility.java

+		reveal = matchedPattern > 0;
+		players = (Expression<Player>) exprs[0];
+		if (reveal && players instanceof ExprHiddenPlayers)
+			targetPlayers = (exprs.length > 1 && !exprs[1].isDefault()) ? (Expression<Player>) exprs[1] : ((ExprHiddenPlayers) players).getPlayers();


@Blueyescat should comment it a bit then, the code is confusing

Pikachu920 · 2018-06-11T19:57:23Z

src/main/java/ch/njol/skript/expressions/ExprHiddenPlayers.java

+		Skript.registerExpression(ExprHiddenPlayers.class, Player.class, ExpressionType.PROPERTY,
+				"[(all [[of] the]|the)] hidden players [(of|for) %players%]",
+				"[(all [[of] the]|the)] players hidden [(from|for) %players%]",
+				"%players%'[s] [all] hidden players");


player's all hidden players isn't grammatically correct, and i think that even player's hidden players sounds awkward so maybe don't have this at all?

Yeah probably. I will remove it and add "by" to the 2nd pattern:

[(all [[of] the]|the)] players hidden (from|for|by) %players%

Pikachu920 · 2018-06-11T20:03:29Z

src/main/java/ch/njol/skript/util/DelayedChangeBlock.java

+	}	
+
+	@Override
+	public BlockState getState(boolean bool) {


how come you changed these methods? this won't work on normal spigot (the method you call does not exist there). Also, you should name the parameter useSnapshot instead of bool if this is kept

Actually i were using Spigot 1.8.8 while testing this PR, i tried again and there is no any error in the console. hmm... Also i don't know how to prevent calling them if they doesn't exist.

You are right about the variable name.

maybe it was removed on newer versions? it isn't in the spigot javadocs, only papers

but that doesn't explain why this change is in this pr anyways

Well they are added in PaperSpigot 1.12.2 but doesn't give error on Spigot 1.8.8 i don't know.

Oh yeah i forgot to explain that, because i used the new hide/showPlayer(Plugin, Player) method that comes with 1.12.2 if it exists 🤷‍♀️

Yeah they don't call a new method just creates.

Nicofisi · 2018-06-16T17:39:03Z

Same here, you need to keep it 2017 for now

This reverts commit 3af27b3.

Snow-Pyon · 2018-07-17T05:56:21Z

src/main/java/ch/njol/skript/conditions/CondCanSee.java

+import ch.njol.skript.doc.Description;
+import ch.njol.skript.doc.Examples;
+import ch.njol.skript.doc.Name;
+import ch.njol.skript.doc.Since;


import order

Snow-Pyon · 2018-07-17T05:56:37Z

src/main/java/ch/njol/skript/effects/EffPlayerVisibility.java

+import org.eclipse.jdt.annotation.Nullable;
+import ch.njol.skript.Skript;
+import ch.njol.skript.doc.Description;
+import ch.njol.skript.doc.Examples;


import order

Snow-Pyon · 2018-07-17T05:56:58Z

src/main/java/ch/njol/skript/expressions/ExprHiddenPlayers.java

+import ch.njol.skript.doc.Since;
+import ch.njol.skript.lang.util.SimpleExpression;
+import ch.njol.skript.lang.SkriptParser;
+import ch.njol.skript.lang.Expression;


and again, import order.

Blueyescat · 2018-07-17T09:09:24Z

Ops i forgot to change the import orders, should be fine now.

Nicofisi · 2018-07-17T12:02:00Z

Whoever merges it, please squash 🙏

Blueyescat · 2018-07-17T14:26:50Z

no no easier to revert

Blueyescat added 2 commits June 10, 2018 23:19

Add player visibility stuff

6b2641b

Remove an unnecessary blank line and fix syntax description

f6110ef

Snow-Pyon suggested changes Jun 10, 2018

View reviewed changes

Blueyescat added 2 commits June 11, 2018 00:09

Fixes

9fe1019

Merge branch 'visibility-stuff' of https://github.com/Blueyescat/Skript…

a5c9ae7

… into visibility-stuff

Pikachu920 reviewed Jun 11, 2018

View reviewed changes

Pikachu920 requested changes Jun 11, 2018

View reviewed changes

Blueyescat added 4 commits June 12, 2018 00:14

Reviews

5a69499

Remove show effect

0de3bfd

Remove an unnecessary import

cd09204

Fix copyrights

3af27b3

Blueyescat added 4 commits June 16, 2018 20:44

Revert "Fix copyrights"

60dc909

This reverts commit 3af27b3.

Remove unnecessary imports

a6dd956

Resolve conflicts with build.gradle

c8dcb26

Merge branch 'master' into visibility-stuff

a841686

Pikachu920 mentioned this pull request Jul 4, 2018

Suggest some additional features for Skript #1291

Closed

24 tasks

Snow-Pyon approved these changes Jul 17, 2018

View reviewed changes

Snow-Pyon suggested changes Jul 17, 2018

View reviewed changes

Fix import orders

b867227

Fix import order

c10dfdc

Snow-Pyon merged commit 60279c0 into SkriptLang:master Jul 17, 2018

Add player visibility stuff #1302

Add player visibility stuff #1302

Conversation

Blueyescat commented Jun 10, 2018 • edited Loading

Snow-Pyon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blueyescat commented Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blueyescat Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blueyescat Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blueyescat Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blueyescat Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicofisi commented Jun 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blueyescat commented Jul 17, 2018 • edited Loading

Nicofisi commented Jul 17, 2018

Blueyescat commented Jul 17, 2018

Blueyescat commented Jun 10, 2018 •

edited

Loading

Blueyescat commented Jun 10, 2018 •

edited

Loading

Blueyescat Jun 11, 2018 •

edited

Loading

Blueyescat Jun 11, 2018 •

edited

Loading

Blueyescat Jun 11, 2018 •

edited

Loading

Blueyescat Jun 11, 2018 •

edited

Loading

Nicofisi commented Jun 16, 2018 •

edited

Loading

Blueyescat commented Jul 17, 2018 •

edited

Loading