Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite attack and damage events #2566

Draft
wants to merge 1 commit into
base: api-12
Choose a base branch
from
Draft

Conversation

Yeregorix
Copy link
Member

@Yeregorix Yeregorix commented Oct 20, 2024

Sponge | SpongeAPI

(I will rebase to API-13 when ready)

This PR attempts to solve long standing issues with the attack and damage events. The previous API was attempting to represent the damage calculation as a chain of mathematical operations which can be modified or replayed with a different base damage. This was problematic because it implies the implementation must be capable to capture all operations to be able to replay. However, due to the nature of modded platforms, it is impossible to capture everything as a mathematical operation. This results in a incorrect final damage value and thus angry pvp players.

Let's take an example: the shield modifier. Modded platforms have a shield event where mods can just set the current damage value. Was it an addition, a multiplication or any other mathematical operation? We can't know. We could just capture it as a constant, but being able to replay with a different base damage becomes useless. We could instead capture a modifier that would re-fire the event each time we replay the chain, however this would certainly break mods that have side-effects.

  1. That's why we must give up the ability to replay everything.

Another problem is that we represent the damage calculation as a chain. First of all, it is not. It has branches, early returns, splits and merges. Even if it was a chain, a mod could do some bytecode modification and add new operations in the middle of the "chain". Since we can't capture these new operations, our representation of the chain will be incomplete.

  1. That's why we must give up the idea that the damage calculation is chain.

Finally, due to having a single event firing after the whole damage calculation is done, the implementation has to cancel all side-effects during the damage calculation, then replay them after the event. Again, this will not work with mod side-effects but even for vanilla this is a brittle and cumbersome approach

  1. That's why it's preferable to modify the "steps" and their side-effects before they actually occur.

Note that even if it seems that these events were mostly problematic on modded platforms (indeed they were completely broken), I have also seen a few mistakes in the vanilla implementation, especially for the invulnerability time mechanic. This is because the API can hardly represent this mechanic correctly.


Now let's talk about the proposed solution.

Forget about the previous concept of DamageModifier and let's introduce the concept of DamageStep which represent a step in the damage calculation that we want to expose to the API. It no longer captures the mathematical operation, but just the values before and after the step. (consequence of 1.)

These steps are just some important key points in the damage calculation, there are not an exhaustive representation of all the things that may happen during the damage calculation. (consequence of 2.)

Now, we split the damage event into two parts: Pre and Post.
Pre happens before the damage calculation and all its side-effects, at this time, we only know the base damage.
Post happens after the damage calculation, all steps have been captured.
During the Pre event, plugins can add some callbacks that will modify the damage value just before or just after a step. These callbacks are called DamageModifier. These callbacks are executed after the Pre event, in the right place during the damage calculation, ensuring that mods and side-effects will adapt to the modified value. (consequence of 3.).

As a bonus, the attack and damage events now share most of their logic in a parent class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant