[lua] allow phys blu spells to inflict added effect on 0 damage

[lapislosh]

I affirm:

• I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
• I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
• I have read and understood the Contributing Guide and the Code of Conduct.
• I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Allows BLU spells to inflict their additional effects even if the spell hits for 0 - this will typically be against Stoneskin, as no spell can do 0 damage just normally. The old code doesn't work properly because it assumes a 0-damage hit is a miss, while the new code explicitly checks to see if any hits landed.

Siknoz test attempting to sleep an earth elemental with stoneskin up:

Steps to test these changes

Find a troll in Mt Zhayolm or anywhere
!mobskill 1744 for diamondhide
Use headbutt or pinecone bomb or something on it and ensure the additional effect lands

[WinterSolstice8]

The vast majority of changes here are

which really doesn't do anything. The ones where you check against hitsDone <= 0 to return early are fine. Can you revert the changes to the files like pictured above?

[lapislosh]

I'm not entirely sure if that works? It will cause OnSpellCast to return both numbers, which I did a quick test before making the changes to see how lua worked

I wasn't sure if that would break whatever might be using the OnSpellCast return value... maybe it's fine? I'm happy to revert if there's no issue.

[WinterSolstice8]

I'll double check how core is handling it, you could be right.

[WinterSolstice8]

You are right in that it returns two parameters back out to core, that would be here (luautils.cppp)

int32 OnSpellCast(CBattleEntity* PCaster, CBattleEntity* PTarget, CSpell* PSpell)
{
    TracyZoneScoped;

    if (PSpell == nullptr)
    {
        ShowError("luautils::OnSpellCast: Spell not found!");
        return 0;
    }

    auto onSpellCast = getSpellCachedFunction(PSpell, "onSpellCast");
    if (!onSpellCast.valid())
    {
        return 0;
    }

    auto result = onSpellCast(PCaster, PTarget, PSpell); // <---- actual call out to lua
    if (!result.valid())
    {
        sol::error err = result;
        ShowError("luautils::onSpellCast: %s", err.what());
        ReportErrorToPlayer(PCaster, err.what());
        return 0;
    }

    int32 retVal = result.get_type(0) == sol::type::number ? result.get<int32>(0) : 0;
    return retVal;
}

However, since it only checks the first (0th) result the 2nd one is discarded. I don't think your change will do anything noticable.

result.get_type(0) fetches the 1st param's type, which we check is a number (in case it is nil, as it can be sometimes)
result.get<int32>(0) will get the result as an int32, which after we check is a number is fine to use

The second result from the lua return would be result.get_type(1), which we don't use. So I don't think the change is really doing anything except explicitly returning only one result.

I suppose in theory this could be faster, but its probably immeasurable. Does that all make sense to you now?

[lapislosh]

Ok, updated with only the 17 phys added effect spells changed now. Tested a bit and didn't see it spitting out any errors due to the extra arg so I think you're correct that it's fine.

[lapislosh]

Hm I'm guessing it's mad about this in Spell.lua which only expects 1 return value, which I don't reaallllyy want to touch for something like this.

---@field onSpellCast? fun(PCaster: CBaseEntity, PTarget: CBaseEntity, PSpell: CSpell): integer?

[WinterSolstice8]

I added a commit to allow that for now. Blue magic is going to get a rewrite/update soonish so I don't expect you to take on the burden of fixing that