fix drop boss bag hook not working properly

[sors89]

#2298, #252

[sors89]

gonna make this pull request draft as i encoutered some errors with HookResult while testing

[SignatureBeef]

Thanks for your PR and apologies for delays, usually don't get time to work on this project.

Have a question in relation to the new cancel code - see review comment.

Typically with OTAPI hooks, "HookResult.Cancel" would not run the event/action, so i currently feel this may need extra adjustments or we consider that changing to DropItemLocalPerClientAndSetNPCMoneyTo0 warrants a relook at how the event occurs.

Few thoughts on the direction, pending additional clarification wrt review comment:

• If we want to still use return -1; some additional IL could be injected to insert a if(num == -1) return;. This will then be able to prevent new items being created, and packets being sent.
• maybe less ideal - change focus from the NewItem call, to around DropItemLocalPerClientAndSetNPCMoneyTo0 instead, and if cancelled, the consumer is responsible for distributing loot (customised, or not).
  e.g. patch instead of modify (not monomod runtime hook), or via the new static hooks (WIP, may be expanded to full coverage)

[SignatureBeef]

i may not be up to scratch with these hooks nowadays, but if it was cancelled, should a new item be created still? And if so, why?

[sors89]

According to how "HookResult.Cancel" tend to do, we should not let the new item be created, and i completely agree with that. Sorry, it was because of my lack of knowledge that i overlooked it.

[sors89]

Thanks for your reply. Don't worry, the delays were not bothered me too much because i had more time to improve my IL editing skill. At the time i submitted this pr, i don't have much experience in this field but somehow the code worked and here we are lol.

• If we want to still use return -1; some additional IL could be injected to insert a if(num == -1) return;. This will then be able to prevent new items being created, and packets being sent.

Ah, this is a good improvement and easy to implement too!

• maybe less ideal - change focus from the NewItem call, to around DropItemLocalPerClientAndSetNPCMoneyTo0 instead, and if cancelled, the consumer is responsible for distributing loot (customised, or not).
  e.g. patch instead of modify (not monomod runtime hook), or via the new static hooks (WIP, may be expanded to full coverage)

A great idea to me. I personally choose this one. Some example code for the function you mentioned would help a lot! And i need some clarifications about the final code to check if i get the idea right:

public static void DropItemLocalPerClientAndSetNPCMoneyTo0(NPC npc, int itemId, int stack, bool interactionRequired = true)
{
      if (HookResult.Cancel)
      {
            CustomWayOfDistributingLoot(npc, itemId, stack, true);                 
      }
      else
      {
            //vanilla code goes here...
      }
}

[sors89]

i'm happy with it

[SignatureBeef]

Thanks for your reply. Don't worry, the delays were not bothered me too much because i had more time to improve my IL editing skill. At the time i submitted this pr, i don't have much experience in this field but somehow the code worked and here we are lol.
According to how "HookResult.Cancel" tend to do, we should not let the new item be created, and i completely agree with that. Sorry, it was because of my lack of knowledge that i overlooked it.

No problems at all and no need to be sorry, we all start somewhere!

A great idea to me. I personally choose this one. Some example code for the function you mentioned would help a lot! And i need some clarifications about the final code to check if i get the idea right:

These were just mainly ideas, but in terms of a patch/hook, just the single line would be enough as you have, however I have also come up with another idea which i think might be the best way:

1. Extend the original hook to process both areas, calling the existing API

    foreach (var csr in new[] {
         modder.GetILCursor(() => (new Terraria.NPC()).DropItemInstanced(default, default, 0, 0, false)),
         modder.GetILCursor(() => Terraria.GameContent.ItemDropRules.CommonCode.DropItemLocalPerClientAndSetNPCMoneyTo0(default, default, default, default))
    })

2. Also retain the new hook for others who want to intercept drop rules outside of this method:

mm.Module.GetType("Terraria.GameContent.ItemDropRules.CommonCode").CreateHooks(mm);

I think this solution will then allow NPC 548 / Drop medals to still function, along side the new hook and subsequently plugins that currently depend on this API.

nb. have granted workflow access, and seems the current stuff doesn't compile.

[sors89]

2. Also retain the new hook for others who want to intercept drop rules outside of this method:

mm.Module.GetType("Terraria.GameContent.ItemDropRules.CommonCode").CreateHooks(mm);

I don't understand this really well. Can you please explain this further?

[SignatureBeef]

2. Also retain the new hook for others who want to intercept drop rules outside of this method:

mm.Module.GetType("Terraria.GameContent.ItemDropRules.CommonCode").CreateHooks(mm);

I don't understand this really well. Can you please explain this further?

This is your existing code which creates "static hooks" for each method within Terraria.GameContent.ItemDropRules.CommonCode.
I think keeping this line will be a great supplement to point 1 above (which effectively addresses this PR), as it will allow plugins/consumers to hook into the other methods, in addition to this classic / API boss bag hook should it not be enough.

[SignatureBeef]

thanks for your contribution, I am planning on including this in the release im working on now. unfortunately though there are some additional changes i would like to see, such as moving back to the original code and location (applying the foreach over the top), however this is a very simple thing to do from my end and to save you the git troubles i will utilise the co-author feature to ensure your work is attributed and appreciated!
if you notice i make a mistake please feel free to reach out, create an issue or another PR.
-L