Feature: OnGiveReputation override for Reputation Modification

[realQuartzi]

Describe your feature request or suggestion in detail

I propose the implementation of a hook system similar to the existing OnGiveXP, but specifically for reputation gain. This new hook, tentatively called OnGiveReputation, would allow developers to intercept and modify the amount of reputation a player gains in various scenarios.

The main use cases for this feature include:

• Customizing reputation gains for specific factions.
• Adding additional conditions or modifiers to reputation rewards
• Easier Logging reputation gains for debugging or analytics purposes.

Describe a possible solution to your feature or suggestion in detail

A potential way to implement this feature is to insert hooks in the following functions:

1. Player::RewardReputation(Unit* victim)
2. Player::RewardReputation(Quest const* quest)

In both cases, right before the GetReputationMgr().ModifyReputation() call, we could introduce a hook that allows scripts to modify the reputation value.

This hook could look like this:
void OnGiveReputation(Player* player, int32 factionId, float& amount, ReputationSource source);

Call Examples:

1. When a player earns reputation (via defeating a unit or completing a quest), the hook is triggered.
2. Scripts can intercept and modify the reputation amount, optionally cancelling or overriding the default behaviour.
3. The final reputation value is passed to ModifyReputation().

That should be close to how OnGiveXP works I believe.

Additional context

Here is an example of how OnGiveReputation could be used:

void OnGiveReputation(Player* player, uint32 factionId, float& amount, ReputationSource source) override
{
    // Is it friday? Cause I want double rep on friday...
    if(CheckIfFriday())
        amount *= 2;
}

Yes very creative :)

[Tondorian]

In ScriptMgr in line 314 is already a method bool OnPlayerReputationChange(Player* player, uint32 factionID, int32& standing, bool incremental);

Is this good enough?

[realQuartzi]

In ScriptMgr in line 314 is already a method bool OnPlayerReputationChange(Player* player, uint32 factionID, int32& standing, bool incremental);

Is this good enough?

Thank you for pointing that out!

While OnPlayerReputationChange does cover some aspects, it lacks the critical element of the ReputationSource. Knowing the source of the reputation gain is essential for implementing some other context-aware logic.
For example, distinguishing whether the reputation comes from a quest, killing a unit, or another action would allow developers to implement specific behaviours tailored to those scenarios.
Unless there is a way to reliably retrieve the ReputationSource within OnPlayerReputationChange, I believe it doesn’t fully provide the flexibility or control needed for some use cases.

If this could be added to OnPlayerReputationChange or there is a method to get this information and be able to still adjust the amount gained, then that would be great.

[Tondorian]

should RepuationSource be a enum or a real class, which contains additional information? For example the unit killed by the player?

[realQuartzi]

should RepuationSource be a enum or a real class, which contains additional information? For example the unit killed by the player?

Fortunately, ReputationSource is already defined as an enum in Player.h (line 237), and it includes various scenarios like kills, quests (daily, weekly, etc.), and spells.

enum ReputationSource
{
    REPUTATION_SOURCE_KILL,
    REPUTATION_SOURCE_QUEST,
    REPUTATION_SOURCE_DAILY_QUEST,
    REPUTATION_SOURCE_WEEKLY_QUEST,
    REPUTATION_SOURCE_MONTHLY_QUEST,
    REPUTATION_SOURCE_REPEATABLE_QUEST,
    REPUTATION_SOURCE_SPELL
};

If we want to expand the hook to include more parameters (like the specific Unit* for kills or Quest const* for quests), it would significantly enhance the overall utility and flexibility of the function. We would then likely require creating multiple OnGiveReputation functions, each tailored to specific sources for example, one for Unit* and another for Quest const*.

I wouldn't mind that at all but then, I am wondering if ReputationSource would be useful for Unit kills at all since we would know the source.

[Tondorian]

I did some digging in the code from my understanding only ReputationMgr is used to call ScriptMgr::OnPlayerReputationChange what makes sense, however to extend the reputation Hook for specific quests/units it is required to refactor a lot of code
At the moment my suggestion would be to move the ReputationSource enum into ReputationManager.h
This makes it easier to use this enum without to include Player.h another option is to use forward declaration of the enum

[realQuartzi]

You bring up a valid point regarding the placement of ReputationSource. However, I’d like to mention that as you know ReputationSource is already defined in Player.h, and the two functions that would require hook calls Player::RewardReputation(Unit* victim) and Player::RewardReputation(Quest const* quest) are also located within Player. They are already nicely in one place.

Since the functions requiring hooks already reside in Player, and ReputationMgr is already reference in Player, it might be valid to move it, as long as it doesn't break anything else. But for the sake of implementing the hooks, I do not believe that it would be necessary to refactor any code at all. Just to place the hooks in the right place.

I am wondering if my suggested solution doesn't work as easily than I am thinking. 🤔

[Tondorian]

this functions need to get the information somehow.

For example Players OnReputationChange is called by the ScriptManager::OnPlayerReputationChange, which is called by ReputationManager::SetOneFactionReputation

The other common used function is SetReputation

however these functions do not have any idea where the reputation was generated, all they do is notifying the ScriptMgr about this event.
The ScriptMgr on its side notify each registered PlayerScript about the reputation change

[realQuartzi]

But the functions Player::RewardReputation(Unit* victim) and Player::RewardReputation(Quest const* quest) literally specify the source.
Unit and Quest. right? The only source not available would be spell. You could theoretically figure out what type of quest it is through the quest itself.
Or am I miss understanding? The end goal is to just adjust the reputation gain amount. Which should be easily achievable in those specific locations.

I personally would not mind if only REPUTATION_SOURCE_KILL and REPUTATION_SOURCE_QUEST were used. This would still be more than what is currently available.

[Tondorian]

you are right, they already exist in player.h I was only looking into PlayerScript.h.
I also found Spell::EffectReputation in SpellEffects.cpp

Sorry I just realized you already pointed to right functions in the beginning, I misunderstood this as suggestion how the hooks should be called