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

Partial Armor Ignore System #659

Closed
wants to merge 18 commits into from
Closed

Conversation

sanek31
Copy link

@sanek31 sanek31 commented Aug 3, 2024

-->

Description

-->

Adds the ability to partially pierce armour.
Example:
Armor with 70% protection, when hit by a bullet with 50% penetration and 20 damage, the final damage will be 13 (as if the protection was 35%).


-->

  • Add a partial armor ignore system
  • [] Some fixes

-->

Media

finnaly.mp4


Changelog

-->

🆑 sanek31

  • add: Added partial armor ignore system.

@github-actions github-actions bot added the Changes: C# Changes any cs files label Aug 3, 2024
@sanek31 sanek31 changed the title Sanek31 1 partial armor ignore system Aug 3, 2024
@SimpleStation14 SimpleStation14 changed the title partial armor ignore system Partial Armor Ignore System Aug 3, 2024
@github-actions github-actions bot added the Status: Needs Review Someone please review this label Aug 3, 2024
@DangerRevolution
Copy link
Contributor

Why would 70% protection, with 50% piercing reduce protection to 35%?

@dge21
Copy link

dge21 commented Aug 3, 2024

because 35 is is 50% of 70

@DangerRevolution
Copy link
Contributor

because 35 is is 50% of 70

It's too early in the morning, sorry lmaoo 😭😭😭😭😭😭

Copy link
Contributor

@DangerRevolution DangerRevolution left a comment

Choose a reason for hiding this comment

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

This needs dual @VMSolidus @OldDanceJacket approval before merge

@DangerRevolution
Copy link
Contributor

test fail valid it seems like?

@@ -125,7 +125,7 @@ public DamageSpecifier(DamageGroupPrototype group, FixedPoint2 value)
/// Only applies resistance to a damage type if it is dealing damage, not healing.
/// This will never convert damage into healing.
/// </remarks>
public static DamageSpecifier ApplyModifierSet(DamageSpecifier damageSpec, DamageModifierSet modifierSet)
public static DamageSpecifier ApplyModifierSet(DamageSpecifier damageSpec, DamageModifierSet modifierSet, float br = 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

the parameter should have a meaningful name.

@@ -165,15 +165,15 @@ public static DamageSpecifier ApplyModifierSet(DamageSpecifier damageSpec, Damag
/// <param name="damageSpec"></param>
/// <param name="modifierSets"></param>
/// <returns></returns>
public static DamageSpecifier ApplyModifierSets(DamageSpecifier damageSpec, IEnumerable<DamageModifierSet> modifierSets)
public static DamageSpecifier ApplyModifierSets(DamageSpecifier damageSpec, IEnumerable<DamageModifierSet> modifierSets, float br = 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the above - those are public methods.

Comment on lines 153 to 160
if (bypassResistance != null)
{
damage = DamageSpecifier.ApplyModifierSet(damage, modifierSet,(float) bypassResistance);
}
else
{
damage = DamageSpecifier.ApplyModifierSet(damage, modifierSet);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (bypassResistance != null)
{
damage = DamageSpecifier.ApplyModifierSet(damage, modifierSet,(float) bypassResistance);
}
else
{
damage = DamageSpecifier.ApplyModifierSet(damage, modifierSet);
}
damage = DamageSpecifier.ApplyModifierSet(damage, modifierSet, bypassResistance?.Value ?? 1f);


if (modifierSet.Coefficients.TryGetValue(key, out var coefficient))
newValue *= coefficient; // coefficients can heal you, e.g. cauterizing bleeding
newValue *= 1-coefficient*br; // coefficients can heal you, e.g. cauterizing bleeding
Copy link
Contributor

Choose a reason for hiding this comment

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

This is sussy and probably will never work as intended, instead making damage heal you.

@Mnemotechnician
Copy link
Contributor

Also, test fails are completely legitimate. You have type errors in your code and something tells me this was not tested...

Copy link
Contributor

@WarMechanic WarMechanic left a comment

Choose a reason for hiding this comment

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

im not a big fan of 'armour pierce' because it ultimately means that a bullet will have a final say in whether someone takes damage. that is to say if you pitted super armour and 100% armour pierce together, armour pierce will always win regardless of how strong the armour was.

really i think we should go for new damage calculation entirely, an 'armour contest' if you will.

@sanek31
Copy link
Author

sanek31 commented Aug 4, 2024

Messed up the code. I'm gonna rewrite it

@DangerRevolution
Copy link
Contributor

I would say... that uh, I think there needs to be an option for % based piercing and value based piercing.

i.e it pierces 5 of the armour value, or something.
or always deal 5 regardless of armour.

@sanek31
Copy link
Author

sanek31 commented Aug 5, 2024

Trying to do it locally , but for some reason it's doesn't works for melee weapons.

@DangerRevolution
Copy link
Contributor

Trying to do it locally , but for some reason it's doesn't works for melee weapons.

You can find any help you need in our Discord if that's useful to you :)

@DangerRevolution DangerRevolution added Priority: 2-High Needs to be resolved as soon as possible Size: 3-Medium For medium issues/PRs Status: Do Not Merge Do not merge Status: Help Wanted Extra attention is needed Type: Feature Creation of or significant changes to a feature labels Aug 5, 2024
@VMSolidus
Copy link
Member

Trying to do it locally , but for some reason it's doesn't works for melee weapons.

Melee weapons work on a different system entirely from guns, and the two have almost no overlap at all.

Copy link
Member

@Pspritechologist Pspritechologist left a comment

Choose a reason for hiding this comment

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

I would probably suggest not taking this route for armour penetration. Generic armour penetration can be added by simply modifying DamageSpecifier to take a 'piercing' field of some kind. This field is used when applying damages and what not with an armour value coming from elsewhere. Adding the piercing value in this way allows you to do it while modifying no other functions or APIs and while automatically adding support backwards for anything that deals damage.

Doing it the way it's being done now is manually adding hardcoded logic for certain weapons, which is certainly not the way forward. This should be generic and added to the usual set of APIs

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the Status: Merge Conflict FIX YOUR PR AAAGH label Aug 10, 2024
@sanek31
Copy link
Author

sanek31 commented Aug 10, 2024

ok

@sanek31
Copy link
Author

sanek31 commented Aug 14, 2024

I'll close for now, I'll make another pull request when I'm done.

@sanek31 sanek31 closed this Aug 14, 2024
@DEATHB4DEFEAT
Copy link
Member

I'll close for now, I'll make another pull request when I'm done.

Reopen this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: C# Changes any cs files Priority: 2-High Needs to be resolved as soon as possible Size: 3-Medium For medium issues/PRs Status: Do Not Merge Do not merge Status: Help Wanted Extra attention is needed Status: Merge Conflict FIX YOUR PR AAAGH Status: Needs Review Someone please review this Type: Feature Creation of or significant changes to a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants