TechnicallyCoded / FoliaLib

This is a wrapper library for aiding in supporting the Folia Paper Fork. This library adds multiple scheduler options to use instead of the Bukkit or Folia native schedulers.
MIT License
70 stars 11 forks source link

runAtEntityLater method broken on Paper only #14

Closed PikaMug closed 3 months ago

PikaMug commented 8 months ago

Thank you for adding c720ba1471f9f60946ce7689d496a229df56b1e8 it works great on Folia!

However, it seems to have broken on Paper, with or without a fallback parameter. FoliaLib 0.3.3 executes the consumer, whereas 0.3.4 simply does not. Furthermore, if a fallback runnable is specified, that gets triggered instead (tested with ProjectileLaunchEvent where the fallback is triggered immediately on launching a Fireball via Player#launchProjectile).

Edit: It appears the Projectile#isValid check returns false on Paper? Perhaps [#isDead](https://hub.spigotmc.org/javadocs/spigot/org/bukkit/entity/Entity.html#isDead()) would be preferable?

Java 17 with Paper version git-Paper-370 (MC: 1.20.4) (Implementing API version 1.20.4-R0.1-SNAPSHOT) (Git: 1fa48d1)

Browsit commented 7 months ago

We get a repeating StackOverflowError when spawning a custom mob which may be caused by this (did not occur on 0.3.3):

[17:35:53 ERROR]: Could not pass event MythicMobSpawnEvent to TestPlugin v1.0
java.lang.StackOverflowError: null
        at org.bukkit.craftbukkit.v1_20_R3.entity.CraftEntity.isValid(CraftEntity.java:389) ~[paper-1.20.4.jar:git-Paper-477]
        at org.browsit.testplugin.libs.folialib.impl.SpigotImplementation.runAtEntityLater(SpigotImplementation.java:236) ~[TestPlugin-1.0-SNAPSHOT.jar:?]
TechnicallyCoded commented 7 months ago

I'm currently away for vacation, sorry for the slow replies. I'll do my best to address this asap. If anyone wants to PR a fix in the meantime, feel free :)

CJCrafter commented 5 months ago

I found this issue when doing my PR for task chains... Please upvote #16 for a fix :+1:

TechnicallyCoded commented 5 months ago

I would never forget about massive issues... :o If the PR is accepted it will be fixed anyway, so I will just leave this open for the time being

Loving11ish commented 4 months ago

@TechnicallyCoded This is still an issue on 0.3.4 too!

PikaMug commented 4 months ago

@TechnicallyCoded This is still an issue on 0.3.4 too!

Already known. You can use the dev branch until 0.4.0 is out.

Loving11ish commented 4 months ago

@PikaMug I've not had to use a dev branch before using maven, how do I change to it please?

Loving11ish commented 4 months ago

@PikaMug I've not had to use a dev branch before using maven, how do I change to it please?

nvm, I figured it out anyway thanks me be a derp lol

Loving11ish commented 4 months ago

Ah dang it, still broken in the dev 0.4.0 version :/

Paper: 1.21-48-master@70b0e84 (2024-07-08T19:27:11Z) (Implementing API version 1.21-R0.1-SNAPSHOT)

Error: https://mclo.gs/AE8niSU

TechnicallyCoded commented 4 months ago

Ah dang it, still broken in the dev 0.4.0 version :/

Paper: 1.21-48-master@70b0e84 (2024-07-08T19:27:11Z) (Implementing API version 1.21-R0.1-SNAPSHOT)

Error: https://mclo.gs/AE8niSU

Will fix today if I can

TechnicallyCoded commented 4 months ago

Thanks to @Loving11ish spam pinging my DMs 40 times and scaring me into order, this is now fixed. Version 0.4.2 coming out (not yet on GH, but available on the repo already) fixes this issue and revamps asyncTeleport support while we're at it.

PikaMug commented 4 months ago

While I'm not getting any errors on 0.4.2 the consumer task is still not executed. Here is the code I'm now testing with on Paper (also applies to runAtEntityLater):

// Fireball is launched via PlayerInteractEvent#getPlayer#launchProjectile(Fireball.class)

@EventHandler
public void onProjectileLaunch(final ProjectileLaunchEvent event) {
    final Projectile projectile = event.getEntity();
    if (!(projectile instanceof Fireball)) {
        return;
    }
    foliaLib.getScheduler().runAtEntityTimer(projectile, task -> {
        System.out.println("This task should be executed"); // does not appear in console at all
    }, () -> {
        System.out.println("But only this fallback is triggered"); // appears in console once upon launch
    }, 2L, 2L);
}
TechnicallyCoded commented 4 months ago

Does it work as intended on Folia? Or is it broken on both?

PikaMug commented 4 months ago

@TechnicallyCoded Just checked, still works on Folia. Forking to check my theory that it should be using !Entity#isDead instead of Entity#isValid but will report back with whatever I find.

PikaMug commented 4 months ago

@TechnicallyCoded I've confirmed that checking whether the entity isDead works as expected on both platforms. As to why the isValid check is treated differently on Folia vs Paper, I cannot say. Let me know if you'd prefer I submit a PR.

TechnicallyCoded commented 4 months ago

The thing is, I am pretty sure isValid checks other stuff too, not just the death state (depending on the entity). So a Projectile instance check + isDead check could be a solution for the time being. This should be very well documented with comments of course, given how random it seems. If you want to PR that, it would probs go quicker. But lmk.