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

Add support for entity final damage callbacks #578

Merged
merged 5 commits into from
Mar 9, 2023

Conversation

Dinorush
Copy link
Contributor

Adds FinalDamageCallbacks to entities.

Final damage callbacks exist for classes and are only used for multiplicative changes to damage to avoid issues where a later damage callback adds/subtracts a flat amount. This is why, for instance, Sword Block uses a final damage callback, as otherwise Railgun's charge damage ignores the damage reduction (as the Railgun callback is added after). Entities don't have these callbacks currently.

Primarily adding this to support better Frontier War harvester damage callback code, but usable elsewhere.

Co-authored-by: uniboi <64006268+uniboi@users.noreply.github.com>
Copy link
Contributor

@NoCatt NoCatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, worked in testing, adding as well as removing callbacks,

Copy link
Contributor

@uniboi uniboi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested by adding and removing FinalDamageCallbacks to an entity and killing it in game.
Works in testing and the code looks good as well

@GeckoEidechse
Copy link
Member

@NoCatt @uniboi did you test backwards compat as well? If not I can do it. Just wanna double check so I don't accidentally make a breaking release by accident ^^"

@NoCatt
Copy link
Contributor

NoCatt commented Mar 9, 2023

@NoCatt @uniboi did you test backwards compat as well? If not I can do it. Just wanna double check so I don't accidentally make a breaking release by accident ^^"

Since its server only, there is no reason it should cause any issues with clients on old versions

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not test / code review PR, simply giving sign-off approval based on other reviews.

@GeckoEidechse GeckoEidechse merged commit b54e038 into R2Northstar:main Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants