Improve fishing #2289

TheLimeGlass · 2019-07-23T14:49:07Z

Description

Improves fishing by adding fishing state, fishing hook, improved event with the fishing state, and getting of the caught entity.

Tested on Paper 1.14.4-138

Test script and new default example script:

on fishing states caught entity and caught fish:
	launch a trailing large ball firework coloured red, purple and lime at entity timed 0
	message "&aCongrats you caught something!" to player

# A fun grappling hook.
on fishing failure and in ground:
	player is holding a fishing rod
	name of player's tool is "&6Grappling hook"
	push player direction from player to fishing hook at speed 2.5

on fish:
	message "You just did something relating to fishing! %fishing state%" to player

TheLimeGlass · 2019-07-23T14:52:08Z

Also I found a memory leak within the EventValueExpression or EnumUtils class, this is probably critical, but when broadcasting the %fishing state% there, it adds up the name of the state every single time. It's not getting a new String list somewhere everytime a new call from the classinfo is ran through it. This probably exists on the other simple enums that utilize this EnumUtil.
https://imgur.com/ueDfC3U
Exists in teleport cause too
teleport cause: https://imgur.com/oIGoCgN
As you know events happen alot, and this can become really bad.
Behavior was different in terms of how it broadcasted because one was used with the event syntax, which is why it stacked in a single string for the first one.

This could also be related #2279

I did not fix this in this pull request, as unrelated

Experience is already handled via the Experience Util and event by the way.

P.S the grappling hook is really fun lol.

@bensku

TheLimeGlass · 2019-07-23T15:05:02Z

Oh ya, and since there are two entities in this event now, the bug where you have to define the player is present in this now.

#1415

FranKusmiruk

Looks good overall.

FranKusmiruk · 2019-07-25T12:49:57Z

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

+@Description("The <a href='../classes.html#entity'>fishing hook</a> involved in a player fishing event.")
+@Examples({"on fishing:", "\tteleport player to fishing hook"})
+@Since("2.4")
+public class ExprFishingHook extends EventValueExpression<FishHook> {


I don't really see the utility of this expression, is it just for documentation? You should be able to use the fish hook or event-fish hook since it's an entity.

No, that's the point of the EventValueExpression is to be able to actually use it in that way. Doesn't work without doing it this way.

EventValueExpression is mainly (and I think only) used as default expression when registering a class and perhaps adding documentation to a specific event value but that's about it. You can use it this way since the ExprEntity handles it.

Being able to use hook instead of fish hook is something I'd rather avoid as well, in case it conflicts with something else in the future.

'fishing' shouldn't be optional. Hook already means a few different things in Skript's source code.

FranKusmiruk · 2019-07-25T12:50:27Z

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

+@Description("The <a href='../classes.html#fishingstate'>fishing state</a> of a player fishing event.")
+@Examples("fishing state is failed or in ground")
+@Since("2.4")
+public class ExprFishingState extends EventValueExpression<State> {


Same thing as the above expression.

FranKusmiruk · 2019-07-25T12:52:08Z

src/main/java/ch/njol/skript/events/EvtFishing.java

+		if (states == null)
+			return true;
+		PlayerFishEvent event = (PlayerFishEvent) e;
+		for (PlayerFishEvent.State state : states.getArray(event)) {


You could use the CollectionUtils#contains method (Skript's CollectionUtils, not Apache's). Doesn't matter that much either way.

Not needed as that would need to go through a separate class.

FranKusmiruk · 2019-07-25T12:53:22Z

src/main/java/ch/njol/skript/events/EvtFishing.java

+	public String toString(@Nullable Event e, boolean debug) {
+		if (states == null)
+			return "fishing";
+		return "fishing with states " + states.toString(e, debug);


The event's syntax doesn't contain with.

It's a debug message

No, it is string representation of the used code. Yes i know what they are used for

It doesn't necessarily represent source code, but in this case I agree.

FranKusmiruk · 2019-07-25T12:56:14Z

Oh ya, and since there are two entities in this event now, the bug where you have to define the player is present in this now.

This problem can be circumvented by registering the entity as a classinfo, though that might be a bit overkill for the matter.

Blueyescat · 2019-07-25T13:03:54Z

Use INSERT VERSION for @Sinces...

I don't feel good about fishing states in the event syntax, they don't look like correct English to me (not all of them).

Also the enum names needs some work, like "on" alias for in_ground (may break the on ground condition), "fishing" what?, "reeling in", "successfully caught" etc

TheLimeGlass · 2019-07-26T04:33:52Z

Use INSERT VERSION for @Sinces...

I don't feel good about fishing states in the event syntax, they don't look like correct English to me (not all of them).

Also the enum names needs some work, like "on" alias for in_ground (may break the on ground condition), "fishing" what?, "reeling in", "successfully caught" etc

on ground and in the ground are not close enough to conflict, and that is the point of using the english.lang so Skript can determine which classinfo type matches.

One is also a condition and the other is a type.

Blueyescat · 2019-07-26T09:46:19Z

Use INSERT VERSION for @Sinces...
I don't feel good about fishing states in the event syntax, they don't look like correct English to me (not all of them).
Also the enum names needs some work, like "on" alias for in_ground (may break the on ground condition), "fishing" what?, "reeling in", "successfully caught" etc

on ground and in the ground are not close enough to conflict, and that is the point of using the english.lang so Skript can determine which classinfo type matches.

One is also a condition and the other is a type.

That is not the subject.

bensku

Looks good. The issues I have with this have been already pointed out by others.

bensku · 2019-08-31T17:47:16Z

src/main/java/ch/njol/skript/events/EvtFishing.java

+	public String toString(@Nullable Event e, boolean debug) {
+		if (states == null)
+			return "fishing";
+		return "fishing with states " + states.toString(e, debug);


It doesn't necessarily represent source code, but in this case I agree.

bensku · 2019-08-31T17:48:24Z

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

+@Description("The <a href='../classes.html#entity'>fishing hook</a> involved in a player fishing event.")
+@Examples({"on fishing:", "\tteleport player to fishing hook"})
+@Since("2.4")
+public class ExprFishingHook extends EventValueExpression<FishHook> {


'fishing' shouldn't be optional. Hook already means a few different things in Skript's source code.

ShaneBeee · 2020-02-28T07:06:10Z

I'm just wondering where we're at with this?
I see there have been a few requested changes, but no action on this PR in about 5 months.

@TheLimeGlass we're you planning on updating based on the requested changes?

ShaneBeee · 2020-05-12T11:49:11Z

Closing due to inactivity.

TheLimeGlass added 2 commits July 23, 2019 08:44

Add fishing

6af0151

Remove duplicate fishing item

4791ff5

Explain the double entity present when sending messages

58f4cdd

FranKusmiruk reviewed Jul 25, 2019

View reviewed changes

bensku requested changes Aug 31, 2019

View reviewed changes

FranKusmiruk added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 17, 2019

ShaneBeee closed this May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve fishing #2289

Improve fishing #2289

TheLimeGlass commented Jul 23, 2019 •

edited

Loading

TheLimeGlass commented Jul 23, 2019 •

edited

Loading

TheLimeGlass commented Jul 23, 2019

FranKusmiruk left a comment

FranKusmiruk Jul 25, 2019

TheLimeGlass Jul 26, 2019 •

edited

Loading

FranKusmiruk Jul 26, 2019

bensku Aug 31, 2019

FranKusmiruk Jul 25, 2019

FranKusmiruk Jul 25, 2019

TheLimeGlass Jul 26, 2019

FranKusmiruk Jul 25, 2019

TheLimeGlass Jul 26, 2019

Blueyescat Jul 26, 2019

bensku Aug 31, 2019

FranKusmiruk commented Jul 25, 2019

Blueyescat commented Jul 25, 2019 •

edited

Loading

TheLimeGlass commented Jul 26, 2019 •

edited

Loading

Blueyescat commented Jul 26, 2019

bensku left a comment

bensku Aug 31, 2019

bensku Aug 31, 2019

ShaneBeee commented Feb 28, 2020 •

edited

Loading

ShaneBeee commented May 12, 2020

Improve fishing #2289

Improve fishing #2289

Conversation

TheLimeGlass commented Jul 23, 2019 • edited Loading

Description

TheLimeGlass commented Jul 23, 2019 • edited Loading

TheLimeGlass commented Jul 23, 2019

FranKusmiruk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheLimeGlass Jul 26, 2019 • 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

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

FranKusmiruk commented Jul 25, 2019

Blueyescat commented Jul 25, 2019 • edited Loading

TheLimeGlass commented Jul 26, 2019 • edited Loading

Blueyescat commented Jul 26, 2019

bensku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneBeee commented Feb 28, 2020 • edited Loading

ShaneBeee commented May 12, 2020

TheLimeGlass commented Jul 23, 2019 •

edited

Loading

TheLimeGlass commented Jul 23, 2019 •

edited

Loading

TheLimeGlass Jul 26, 2019 •

edited

Loading

Blueyescat commented Jul 25, 2019 •

edited

Loading

TheLimeGlass commented Jul 26, 2019 •

edited

Loading

ShaneBeee commented Feb 28, 2020 •

edited

Loading