Additional events and NPC:collect

[MrDoubleA232]

Adds the following events: onBlockRemove(eventObj, block, makeEffects), onBlockDelete(eventObj, block), onNPCCollect(eventObj, npc, playerObj) and their respective "post" versions. Also adds NPC:collect(playerObj).

I'm glad I could just reuse onBlockHit's code for most of that.

Requires the following changed lua files: ffi_npc, ffi_block, and main_events.

If this is accepted, npcManager.collected should be deprecated and anything that uses it should be replaced with onPostNPCCollect.

[Bluenaxela]

I assume onNPCCollect maps to the original code's "TouchBonus" function? I notice that function has a variety of checks in it a few different levels that may prevent the collection from actually occurring, potentially in some reasonably common cases. This means the onNPCCollect event may get get false alarms due to where you have the hook. As such, I wouldn't quite consider it ready for prime time. Maybe if it was renamed to make it clear the event represents that it is trying to be collected, it would work? Unsure. It's a problem as it stands though, and if we wanted an event in the future that's more precise we wouldn't want this one being confused with it.

I wonder if allowing onBlockRemove to be canceled is wise. It seems like the sort of thing that could lead to awkward jank. A few of the cases that do things with block removal do something with the block after. Might also be some awkward cases with canceling onBlockDelete and some Lua code too.

I'm also not sure if the onBlockRemove/onBlockDelete dichotomy makes sense to expose in an event like that. They're fairly alike cases with the main distinction being that delete allows it to be reused by Block.spawn, which is relevant to code that is spawning and deleting blocks, but I'm unsure that this is a safely actionable distinction for other code trying to react to a block being removed/deleted. Need to give that some thought.

[MrDoubleA232]

After looking at the conditions for TouchBonus, the only major check I found was this bit at the top. The only other condition I saw that would cause it to completely fail was trying to touch the fairy pendant in a clown car, so it seems reasonable to move onNPCCollect and NPC:collect down to past that major check.

As for cancelling the onBlockX functions... well, I had been wanting these for a while, but I did do them specifically because I need to cancel them for something (though the sanity of that... is still up for question, perhaps). Also, maybe it would be reasonable to merge onBlockRemove and onBlockDelete, but then have an argument to distinguish them?

[Bluenaxela]

The bit of the top is most of it. Taking a closer look, looks like indeed the fairy pendant is the only other case that prevents the NPC from being killed. There are a few cases that prevent the collection from doing anything besides kill the NPC, but causing the NPC's death seems sufficient to count as collected I'd say. I'll note that one option besides moving the hook, would just be to duplicate the logic that leads to those two cases where it does nothing.

Hmm, an argument to distinguish might make sense. Either way I feel like it's probably an event that might warrant some clear warnings in documentation, and at least a suggestion to be careful about canceling it since there's certainly a range of possible causes, where canceling would have differing implications.

[MrDoubleA232]

Well, those false alarms should be fixed (including the fairy pendant case), and there's a slightly updated ffi_npc. I suppose I can go with the merging of the two block events plus a warning in documentation?

[MrDoubleA232]

Merged the two and gave it an extra argument. New ffi_block and main_events.

[Bluenaxela]

Ah, one other notable issue: LunaLuaNPCCollect is not safe. It has the potential to crash if that FFI call gets JIT'ed. Calling native_collectNPC will lead to triggering onNPCCollect, which is... very bad. One crucial rule of LuaJIT FFI function calls, is nothing downstream from the FFI call that gets compiled by the JIT can ever be allowed to alter the state of Lua (meaning, no using the Lua C API, including via Luabind). Therefore it must strictly be the case that no FFI call ever leads to a Lua event.

LuaJIT kinda-ish tries to automatically handle this, but it's imperfect. It'll automatically blacklist a C function for JIT'ed calls if it detects that function passes control back to Lua. The trouble is, if the call doesn't always pass control back to Lua, then the JIT may assume it's always safe, and this will lead to a crash if the FFI call gets compiled and subsequently tries passing control back to Lua.

There are a few ways to fix this. One is to implement the function via Luabind instead. Another is to disable compilation of that FFI call via jit.off(...).

(As a note, helping detect this is why many FFI functions contain the line CLunaFFILock ffiLock(__FUNCTION__); which performs a check to ensure no Lua events get triggered from within that FFI function and give an error rather risk a confusing crash)

[MrDoubleA232]

I'm guessing using Luabind is just like how the existing :HarmCombo and whatnot work, right? If so, that seems the easiest.

[Bluenaxela]

Yeah, HarmCombo is a reasonable example.

[MrDoubleA232]

Well, should be fixed now! I just did it pretty much the exact same way as HarmCombo. Updated ffi_npc.