Skript icon indicating copy to clipboard operation
Skript copied to clipboard

🚀 Continue on Fishing event

Open AyhamAl-Ali opened this issue 4 years ago • 10 comments

Description

This PR is the continue of work of @DeltaRays's PR #3992 with his permission

:heavy_check_mark: Added fishing state :heavy_check_mark: Added fishing hook :heavy_check_mark: Added caught entity :heavy_check_mark: Added dropped XP of fishing event

:heavy_check_mark: Added condition hook is in open water (seems to be bugged from spigot as it always return true even when in 1x1x1 water block or no water at all) :heavy_check_mark: Added pull hooked entity effect :heavy_check_mark: Added bucket entity event + expression :heavy_check_mark: Added fishing lure expr :heavy_check_mark: Added hooked entity expr :heavy_check_mark: Added hook waiting time


Target Minecraft Versions: Any Requirements: None (MC 1.14+ for reel in, 1.17+ for BucketEntity event) Related Issues: #3992

AyhamAl-Ali avatar Aug 16 '21 20:08 AyhamAl-Ali

Perhaps out of the scope of this pull request, but it seems really straightforward to add:

While these expressions are probably way out of Skript's scope, I think it is a good opportunity to finally support everything fish-related. Of course, the last 2 classes are very specific. I'll let the team decide whether or not to add these as expressions.

Edit: for the sake of completion, I will put this in here as well:

Mwexim avatar Aug 28 '21 12:08 Mwexim

Perhaps out of the scope of this pull request, but it seems really straightforward to add:

While these expressions are probably way out of Skript's scope, I think it is a good opportunity to finally support everything fish-related. Of course, the last 2 classes are very specific. I'll let the team decide whether or not to add these as expressions.

Edit: for the sake of completion, I will put this in here as well:

Good suggestions :+1: however, I prefer to do these in another PR to make it clean since this PR had so many work already and the suggestions contain a good amount of new features.

AyhamAl-Ali avatar Aug 28 '21 17:08 AyhamAl-Ali

Good suggestions :+1: however, I prefer to do these in another PR to make it clean since this PR had so many work already and the suggestions contain a good amount of new features.

I feel like the event should be added. The rest can be added in another pull request indeed. Great work so far!

Mwexim avatar Aug 28 '21 17:08 Mwexim

I feel like the event should be added. The rest can be added in another pull request indeed. Great work so far!

Oh this event is fish related, I thought it is bucket filling or something. Sure I will look into that and add it. Thank you :)

AyhamAl-Ali avatar Aug 28 '21 17:08 AyhamAl-Ali

Should we also include a FishingHookState expression? I believe this should be fine since we have FishingState as well.

Fusezion avatar Aug 27 '22 00:08 Fusezion

also when using fishing state how does it return since my version of this was returning fishingstate.(STATE) due to a missing part within the .lang file for fishing states


the same can be said about how fishing hook returns when sent toString

Fusezion avatar Aug 28 '22 13:08 Fusezion

also when using fishing state how does it return since my version of this was returning fishingstate.(STATE) due to a missing part within the .lang file for fishing states

the same can be said about how fishing hook returns when sent toString

Do you have custom lang files placed in your Skript folder? that might not have been updated, try to either delete Skript folder (backup ofc) or try on a fresh server and let me know

AyhamAl-Ali avatar Aug 28 '22 13:08 AyhamAl-Ali

also when using fishing state how does it return since my version of this was returning fishingstate.(STATE) due to a missing part within the .lang file for fishing states the same can be said about how fishing hook returns when sent toString

Do you have custom lang files placed in your Skript folder? that might not have been updated, try to either delete Skript folder (backup ofc) or try on a fresh server and let me know

I did a custom skript addon for testing which would probably be the cause just wanted to make sure it didn't cause the same issue, but if it wasn't an issue for you then I don't need to worry about it

Fusezion avatar Aug 28 '22 13:08 Fusezion

Hey @AyhamAl-Ali, can I ask you to try something for me this is my code which should be the same as your version

on fishing:
	if fishing state is fishing:
#		set apply lure of fishing hook to false
		set maximum waiting time of event-fishing hook to 5 seconds

I'd like you to try this with 2 types of fishing rod and apply lure commented and uncommented by default it's always true so only need to set it to false never true

I'd like 1 fishing rod to have lure and 1 plain fishing rod

While I was testing this on my server if I have apply lure set to true and edited the maximum waiting time the fishing rod with lure doesn't work ever as in minecraft doesn't attempt to ever try to collect a fish

Fusezion avatar Aug 29 '22 04:08 Fusezion

Loving to see how it's coming along. Additionally didn't notice this until recently but it seems like Bukkit added more stuff into FishHook class such as lureTime and lureAngle?

Both of which I believe should be added before merging, won't prevent the approval tho

https://hub.spigotmc.org/javadocs/spigot/org/bukkit/entity/FishHook.html

Includes isSkyInfluenced, isRainInfluenced, lureTime and lureAngle All of which seemed added in 1.19, additionally included them finally adding setWaitTime(int) which paper supported prior to bukkit adding, paper added theirs in 1.18

Lure angle seems related to a rotation around the hook in which the fish can come from.

Good suggestions :ok_hand: I will keep that in mind to create another PR for it, this PR is big enough that it will take long time to review, I don't plan to delay it any more :D

AyhamAl-Ali avatar May 20 '23 05:05 AyhamAl-Ali