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

Armour Refactor #18547

Merged
merged 28 commits into from
Aug 1, 2022
Merged

Armour Refactor #18547

merged 28 commits into from
Aug 1, 2022

Conversation

Edan52
Copy link
Contributor

@Edan52 Edan52 commented Jul 24, 2022

What Does This PR Do

Applies the idea proposed in #18521

Refactors the armour calculation & modifies some of the existing armour values to be more in line with these changes. This does make the code a bit harder to understand initially due to the higher complexity of the calculation, I do intend to do what I can to explain what's happening (updating the armour wiki, providing tools).

This means that individual armour pieces should remain effectively unchanged, within a couple percent.

Armour code is scattered in many places, so please feel free to comment or ping me on discord with anything I might have missed.

Armour penetration now comes in two forms, percentage and raw. Percentage is calculated first and removes a percentage of your total armour, and then the raw armour is taken away. For example 75 armour hit by a 30% armour penetration followed by 20 raw penetration would look like this:

75 * 0.7 = 52.5 armour
52.5 - 20 = 32.5 final armour.

Percentage is not implemented yet, but the framework is there for items and simple mobs and it'd be good to go over armour penetration values either in this PR or another one as there are some potential issues that could rise if they remain unchanged.

Shield code has also been cleaned up and now prioritises the item you're holding that gives the highest block chance, rather than just your left hand and then right hand.

What does this mean for your every day round?

Security jumpsuit + sec armour now results in 25 melee armour which is a 33% melee damage reduction, rather than the 40% it was before. The rest of the damage types remain effectively unchanged.

Tactical turtleneck + blood-red hardsuit results in 45 melee armour, which is about a 47% reduction in melee damage, whereas before it would have been 50%

Tactical turtleneck + elite syndicate hardsuit results in 80 melee armour, which is about a 62% reduction in melee damage, where it would've been 70% before.

Mining armour stacking has been nerfed quite a bit, you won't be able to reach much more than 70% reduction now.

All I have done thus far is change the values to be the same as they were before, so some common combinations of armour have most likely also been nerfed. If requested, I could buff these to be more equal to what they were before however I'd like to keep the refactor atomic to that.

Armour penetrations have been tweaked by me and @hal9000PR to be around about as effective in combat as they were before, there is no simple calculation to convert the values so we've had to consider what weapons are used in which situations against which armour and assign values accordingly.

Please leave comments relevant to the code on this PR, talk about the idea on the discussion thread.

Why It's Good For The Game

See reasoning on the project discussion.

Changelog

🆑 CornMyCob
tweak: Armour now uses a new calculation, stacking lots of armour is now less effective.
tweak: Shields now prioritise the item you're holding that has the highest block chance.
/:cl:

@ParadiseSS13-Bot ParadiseSS13-Bot added the -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally label Jul 24, 2022
@Qwertytoforty
Copy link
Contributor

Qwertytoforty commented Jul 24, 2022

I am not sure I am a fan of this formula, that makes armor under 50 stronger and over 50 weaker. I feel like addressing the sources of stacking, sec jumpsuit, armored accessories, and bone bracers, may be better than this. This means we can keep the armor values simple to read, with reading 20 melee being a 20% reduction, vs actually being ~29% reduction. Also keeps it more consistent with how the rest of the armor works in the game. Alternatively, we make stacking armor items multiplicative, vs additive.

@AffectedArc07 AffectedArc07 added the Balance This PR will modify how effective something is or isnt label Jul 24, 2022
@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting approval This PR is waiting for approval internally and removed -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally labels Jul 24, 2022
@Rurik123
Copy link

Rurik123 commented Jul 28, 2022

I'm an idiot when it comes to this sort of thing, so feel free to disregard/correct this if I've made a mistake.

If I'm reading this right, the armor value for anything above 50 is nerfed, even if it is not relying on stacking armor. Like Elite Syndicate HS being lowered from 60 soley cause it stands alone above 50, not cause of any additional armor.

Wouldn't it be superior if the formula made the largest armor value flat, and instead adjusted the efficiency of the smaller stacking armor values? This way the established armor value of inquis/ERT/syndicate/etc armor is kept, as well as the armor stacking problem being solved?

Apologies if I misread or misunderstand something.

Edit - Concern Addressed

@SpringSkipper
Copy link
Contributor

The current armor system is entirely fine except for the additive stacking.

If you change the formula then you're taking it on yourself to change the armor amount of every armor item in the game. That seems like a hard requirement to me. Otherwise you're blindly nerfing and buffing a lot of items without regard to the game effect those changes will have.

Changing stacking to multiplicative would work just as well at fixing the stacking mining armor problem, plus it would remove the requirement to readjust every armor value in the whole game.

@ParadiseSS13-Bot ParadiseSS13-Bot added the Testmerge Requested This PR has a pending testmerge request label Jul 28, 2022
@Edan52 Edan52 marked this pull request as ready for review July 28, 2022 22:56
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Jul 29, 2022
code/datums/weather/weather_types/radiation_storm.dm Outdated Show resolved Hide resolved
code/__DEFINES/armour.dm Outdated Show resolved Hide resolved
code/game/gamemodes/cult/cult_items.dm Show resolved Hide resolved
code/game/objects/items.dm Outdated Show resolved Hide resolved
code/modules/mob/living/simple_animal/hostile/syndicate.dm Outdated Show resolved Hide resolved
code/modules/mob/living/simple_animal/simple_animal.dm Outdated Show resolved Hide resolved
code/modules/projectiles/projectile/magic.dm Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Jul 31, 2022
@hal9000PR hal9000PR removed the Merge Conflict This PR is merge conflicted label Jul 31, 2022
@ParadiseSS13-Bot ParadiseSS13-Bot added Testmerge Active This PR is currently testmerged on production -Status: Awaiting review This PR is awaiting review from the review team and removed -Status: Awaiting approval This PR is waiting for approval internally labels Jul 31, 2022
@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting merge This PR is ready for merge and removed -Status: Awaiting review This PR is awaiting review from the review team labels Aug 1, 2022
@hal9000PR hal9000PR merged commit 1d552ab into ParadiseSS13:master Aug 1, 2022
github-actions bot added a commit that referenced this pull request Aug 1, 2022
@Edan52 Edan52 mentioned this pull request Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-Status: Awaiting merge This PR is ready for merge Balance This PR will modify how effective something is or isnt Testmerge Active This PR is currently testmerged on production Testmerge Requested This PR has a pending testmerge request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants