PRE_X_STATUS_APPLY & POST_X_STATUS_APPLY

[BabyblueSheep]

Resolves #549 by adding 32 callbacks, 2 for each of the following status effects: Bait, Bleed, Brimstone Mark, Burn, Charm, Confusion, Fear, Freeze, Knockback, Magnetize, Midas Freeze, Petrification, Poison, Shrink, Slow, Weakness.

MC_PRE_BURN_STATUS_APPLY and MC_PRE_POISON_STATUS_APPLY let you return a table with the following entries: { int duration, number damage }.
MC_PRE_KNOCKBACK_STATUS_APPLY lets you return a table with the following entries: { int duration, Vector pushDirection, boolean takeImpactDamage }.
MC_PRE_SLOWING_STATUS_APPLY lets you return a table with the following entries: { int duration, number slowValue, Color slowColor }.
MC_PRE_CONFUSION_STATUS_APPLY lets you return a table with the following entries: { int duration, boolean ignoreBosses }.
The 5 mentioned callbacks, as well as all of the other PRE_X_STATUS_APPLY callbacks, also let you return either an integer to change duration, or a boolean to cancel the effect. Additionally, you can set an EntityType as the optional argument.

All of the PRE_X_STATUS_APPLY and POST_X_STATUS_APPLY callbacks have parameters that match the status effect function signatures.

[ConnorForan]

Hi, I apologize for the delay in this being reviewed

We'd like to pitch a potential approach for folding all of this into one pair of callbacks, MC_PRE_STATUS_APPLY and MC_POST_STATUS_APPLY, instead of 32 callbacks:

• New StatusEffect enum to identify the individual statuses (ie, StatusEffect.POISON).
• The callbacks pass (StatusEffect, Entity entity, EntityRef source, int duration, extraParam1, extraParam2)
• The optional param of the callbacks is the StatusEffect enum
• The types of the extra params must be inferred based on the StatusEffect
• MC_PRE_STATUS_APPLY accepts a return value of either an integer to change the duration, a boolean to prevent the status, or a table like {int duration, extraParam1, extraParam2} to modify the extra params

While this does make the type/purpose of the extra params ambiguous and dependent on the status, this does provide some benefits:

• 2 callbacks vs 32 feels nicer for documentation and maintainability purposes.
• While this means you can't use EntityType as the optional param, if you wanted special handling for all statuses for a specific entity, it could be nicer to have a single callback instead of 16. Performance shouldn't be a concern for these.
• You could potentially imagine a mod manually running MC_PRE_STATUS_APPLY for custom statuses by passing a string in place of the StatusEffect and whatever they want for the extra params. Not a necessary feature by any means, but its a neat bit of flexibility that doesn't require any additional work.
• If your mod adds a callback using a specific StatusEffect as the optional param, you can still just give the params their correct names, like (status, entity, source, duration, pushDir, takeImpactDamage)

I don't believe that doing something like this would require an absurd amount of changes to the PR, hopefully at least. Most of the C++ side handling should be the same, they just all go through the same callbacks. Though type checking would either need to be done C++ side, or via a custom callback handling function in main_ex.lua (if we ever wanted to make it so that returning in this callback still additively runs the callbacks of other mods after it, would maybe need to be the latter)

In any case this is all based on a conversation I had with some other devs, and our opinions. What do you think?

[BabyblueSheep]

Hi.

I do mostly agree and I probably will merge the callbacks into just 2 some time later, though I probably won't try making MC_PRE_STATUS_APPLY and MC_POST_STATUS_APPLY work with strings for the StatusEffect. At least, not until I or someone else implements a way of storing and applying custom status effects, though I doubt refactoring the callback implementations to account for custom status effects will be hard.

[ConnorForan]

Don't worry about the strings thing - I dont think you would really need to do anything in particular to allow that to work anyway. In the hypothetical I mentioned the callback would be running entirely lua-side. But yeah its also not remotely a needed feature, just a neat thought.

So yeah, dont worry about that bit. Glad you generally agree with the suggestion!

[jsgnextortex]

yea, just use an enum, if we ever decide to add support for custom effects and whatnot we can just give them an id, like it works for other things.

[BabyblueSheep]

Folded all callbacks and added manual type checking and additive running for the 2 callbacks.
Additionally, added the callbacks and the new StatusEffect enum to documentation and changelogs.

[BabyblueSheep]

Oh, wait, are changes to documentation regarding future additions meant to go in a different branch?

[jsgnextortex]

Ye, the documentation shouldnt be pushed straight to main, was going to say, otherwise it will instantly show up on the docs and people wont be able to use it yet and be confused.

[BabyblueSheep]

NOW I think it's done.

[ConnorForan]

You might find having changelog.txt in PRs can get annoying since it is a common case for merge conflicts

But it's up to you, if you dont mind fixing the merge conflict you can leave it in and keep it updated. The alternative would be to edit the changelog afterwards

[BabyblueSheep]

Resolved requested changes.

[BabyblueSheep]

Should probably mention that I resolved requested changes? Unsure how GitHub notifications work.

[ConnorForan]

Yes I have seen all of these notifications, I have just not had time to review this today. I apologize but please be patient.

[ConnorForan]

Looks good to me, thank you for making the changes to unify these into a single callback, I'm happy with how this works now, good work.

Sorry for the delay and any headaches with the typechecking - the typechecks and the setup for errors/warnings have been due for a bit of a refactor, but that'll be for another day.

I will merge in this tomorrow if there are no other comments, thanks again!