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

Armor system #370

Open
wants to merge 42 commits into
base: master
from

Conversation

@TimGoll
Copy link
Member

commented Aug 11, 2019

I made a simple implemenatation of a new armor system based on Histaleks idea. The aim of the new system is to make a visually easy to understand armor system that is not confusing when multiple armor effects stack. The percentual system right now is confusing too many players, especially when you have for example the classic armor in combination with a super soda.

  • armor is a secondary health information on top of the normal HP
  • it blocks only bulletdamage
  • 20% of the damage are sucked into the armor and reduce the armor value, 10% vanish and 70% get reduced from the health (values open for discussion)
  • the classic armor item is replaced with a multibuy armor item, that instantly grants 30 armor to the player
  • other roles can set specific values if the roledev plans on giving roles armor on spawn
  • heroes roles and supersodas can use this system too instead of the classic armor
  • classic armor is included as a nonbuyable fallback option
  • right now only pureskin support is included and the armor bar is hidden if a player has no armor

@TimGoll TimGoll added the WIP label Aug 11, 2019

@TimGoll TimGoll requested review from Alf21, lebroomer and saibotk Aug 17, 2019

@TimGoll

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

Why this armorsystem?
The armor system in TTT is always a bit confusing, especially if you have multiple addons that reduce the damage taken. This approach changes this.

Are there any drawbacks?
No. If players do not like the new system, they can disable it with a convar.

How does it work?
Additional to the HP points, each player now has armor points. When he receives damage, the damage gets split between the armor and the HP (20% armor, 70% HP, 10% vanish, values tweakable). If a player has more that 50 armor, the values increase by about 10%. This allows for stronger armor if a player buys the armor item twice, or finds multiple armor sodas for example.


It is basically finished now, it should be tested in a gameplay environment to verify the new armor values. Some tweaks might be necessary.

@TimGoll TimGoll removed the WIP label Aug 17, 2019

@Alf21

This comment has been minimized.

This should be cached too...

This comment has been minimized.

Copy link
Member Author

replied Aug 17, 2019

this was not done by me, but yes it should. But it tested it and there was no performance difference

@Alf21

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

I like the idea, but I think there is a better solution than defining a fixed constant like "50" armor.
In my opinion, the currently stored armor should be used for the calculation relatively. Defining 50 as a fixed constant and maybe 100 as a maximum value seems to be inconsistent because there isn't a maximum value for the health. 100 is the default health amount, but there should be the possibility to have a higher value always. As a second argument, restricting the armor amount to 100 doesn't make sense thinking about the issue that you are able to protect yourself with new technologies in real life better (more than the currently defined "100%").
In conclusion, there could never be a fixed or defined 100%. As a result, there can't (or shouldn't) be a fixed value like "50".

gamemodes/terrortown/gamemode/client/cl_status.lua Outdated Show resolved Hide resolved
gamemodes/terrortown/gamemode/client/cl_status.lua Outdated Show resolved Hide resolved
lua/terrortown/entities/items/item_ttt_armor.lua Outdated Show resolved Hide resolved
@TimGoll

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

100 is not the max. It's the same as with the health. Max gets used if you want to draw a bar, but it is not capped to 100

Should I add a convar for the reinforced armor? Or do you want function that calculates the protection based on the value? I considered the latter, but ditched it because I think it will confuse players

@TimGoll

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

Additionally I'd like to add that armor is no percentual value

@lebroomer

This comment has been minimized.

Copy link
Collaborator

commented Aug 17, 2019

I think a percental value could be ambigous, since stacking armor is a possibility. With a percentual value the new reinforced border would be complicated, where as an additive procedure with "50" always being the reinforced border would be simple and understandable.

@Alf21

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

I don't think that the current solution is understandable and reasonable. You need to use a formula to calculate the damage. Same as using the armor as a relative factor. The reinforced armor thing is a nice idea too, but maybe a thing that makes it more complicated (2 different formulas). Displaying the current armor and using a constant formula is (in my opinion) the easiest way in this armor system.

@@ -460,13 +463,17 @@ end
-- @return boolean success?
-- @realm server
function plymeta:GiveEquipmentItem(id)

This comment has been minimized.

Copy link
@Alf21

Alf21 Sep 1, 2019

Member

What's the difference between ply:GiveEquipmentItem and ply:AddEquipmentItem? Seems to be inconsistent after giving a deeper look...

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 2, 2019

Author Member

Very good question. I was confused too, since they are basically the same with the same checks. But they are not done by me and maybe something relies on them. Therefore I did not want to remove any of them and just made sure that both of them are working

@Alf21 Alf21 dismissed their stale review Sep 1, 2019

unchanged requests

@Alf21 Alf21 self-requested a review Sep 1, 2019

@saibotk
Copy link
Member

left a comment

All in all, the armor system would add up nicely with the other additions to TTT, but i marked some things, which i suppose to be changed / discussed prior to this being considered merge ready.

Additionally, since you cannot comment on files etc:

  • Language strings are not selected based on which mode (classic armor system) is enabled.
  • Is Item.limited is checked properly on serverside?
  • Changes to the Status system do not belong here.
  • Removal of hud_radar.png also does not belong to the Armor system.
@@ -18,6 +18,8 @@ if SERVER then
resource.AddFile("materials/vgui/ttt/dynamic/roles/icon_traitor.vmt")
resource.AddFile("materials/vgui/ttt/dynamic/roles/icon_det.vmt")
resource.AddFile("materials/vgui/ttt/dynamic/roles/icon_no_team.vmt")

resource.AddFile("materials/vgui/ttt/equip/reroll.png")

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 1, 2019

Member

No pngs, and this does not belong to the armor system.

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 2, 2019

Author Member

I just added this to the downloadlist, it has nothing todo with the armor system, but I commited it to the wrong branch. I can change it though

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 2, 2019

Author Member

Hmm, I changed it to a vmt/vtf, now it is the only icon of the shop tabs that is a vmt/vtf. All the others are pngs. Therefore I think it should stay a png.

-- Item control
if ply:HasEquipmentItem("item_ttt_radar") then
	local dradar = RADAR.CreateMenu(dsheet, dframe)

	dsheet:AddSheet(GetTranslation("radar_name"), dradar, "icon16/magnifier.png", false, false, GetTranslation("equip_tooltip_radar"))
end

-- Weapon/item control
if IsValid(ply.radio) or ply:HasWeapon("weapon_ttt_radio") then
	local dradio = TRADIO.CreateMenu(dsheet)

	dsheet:AddSheet(GetTranslation("radio_name"), dradio, "icon16/transmit.png", false, false, GetTranslation("equip_tooltip_radio"))
end

-- Credit transferring
if credits > 0 then
	local dtransfer = CreateTransferMenu(dsheet)

	dsheet:AddSheet(GetTranslation("xfer_name"), dtransfer, "icon16/group_gear.png", false, false, GetTranslation("equip_tooltip_xfer"))
end

-- Random Shop Rerolling
if credits > 0 and GetGlobalInt("ttt2_random_shops") > 0 and GetGlobalBool("ttt2_random_shop_reroll") then
	local dtransfer = CreateRerollMenu(dsheet)

	dsheet:AddSheet(GetTranslation("reroll_name"), dtransfer, "vgui/ttt/equip/reroll.vmt", false, false, GetTranslation("equip_tooltip_reroll"))
end

https://github.com/TTT-2/TTT2/blob/master/gamemodes/terrortown/gamemode/client/cl_equip.lua#L679

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 5, 2019

Member

Wether or not, this should be moved out of this PR

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 5, 2019

Author Member

Since I did not change anything besides adding a line to the forcedownload list, I see no point in removing it here and adding it manually to the master.

Yes, it does not belong in here, but adding two commits for one line of code seems pointless to me

resource.AddFile("materials/vgui/ttt/perks/hud_disguiser.png") -- disguiser
resource.AddFile("materials/vgui/ttt/perks/hud_radar.png") -- radar

resource.AddFile("materials/vgui/ttt/perks/hud_armor.png") -- armor HUD
resource.AddFile("materials/vgui/ttt/perks/hud_armor_reinforced.png") -- armor reinforced HUD

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 1, 2019

Member

No pngs too

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 2, 2019

Author Member

no. All items and status effects use pngs. Alf or you decided this.

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 5, 2019

Member

then we should change this

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 5, 2019

Author Member

why though? there's only a performance difference on initial material creation and we have already about 30 icons in TTT2 and heroes.

gamemodes/terrortown/gamemode/client/cl_status.lua Outdated Show resolved Hide resolved
gamemodes/terrortown/gamemode/client/cl_status.lua Outdated Show resolved Hide resolved
@@ -81,7 +81,9 @@ if CLIENT then
surface.SetDrawColor(item.hud_color.r, item.hud_color.g, item.hud_color.b, math.Round(factor * 255))
surface.DrawRect(pos.x, curY, size.w, size.w)

util.DrawFilteredTexturedRect(pos.x, curY, size.w, size.w, item.hud, iconAlpha, fontColor)
local hud_icon = item.hud.GetTexture and item.hud or item.hud[item.active_icon]

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 1, 2019

Member

Why is this in here?

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 2, 2019

Author Member

I see ne reason why it shouldn't be here?

Edit: In case you are irritated by item.hud.GetTexture: let me explain. The reworked status system allows multiple icons for the same status to be used. But I had to keep compatibility with the "old" version. item.hud.GetTexture checks if it is a material.

But while thinking about this, I think I can remove this at this place, since I have this check already in RegisterStatus

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 2, 2019

Author Member

It has to be this way since the same function is used for items and statusicons. Therefore this check has to be made

an alternative would be to allow items to have multiple icons too.

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 5, 2019

Member

This also belongs to your status system etc this should be done in a separate PR

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 5, 2019

Author Member

no, this was done to make the armor system work. Without this, the armor system does not work

gamemodes/terrortown/gamemode/server/sv_player.lua Outdated Show resolved Hide resolved

-- before the karma is calculated, but after all other damage hooks / damage change is processed,
-- the armor system should come into place (GM functions are called last)
HandlePlayerArmorSystem(ent, infl, att, amount, dmginfo)

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 1, 2019

Member

Could you explain, why this is excluded from the previous if statement?

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 2, 2019

Author Member

Because I wanted to keep it a bit seperated to not clutter the code above. Additionally I like to work with if bla then return end instead of nested ifs

gamemodes/terrortown/gamemode/server/sv_player.lua Outdated Show resolved Hide resolved
@TimGoll

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

@saibotk Thanks for the feedback. Except one or two comments I agree with your points.

Language strings are not selected based on which mode (classic armor system) is enabled.

yep, I'll change that

Is Item.limited is checked properly on serverside?

I implemented and tested it client and serverside. It worked for me. I'll ask Nick to test this on his server.

Changes to the Status system do not belong here.

yes and no. You're right, but it would have been a nightmare to make this in two different PRs.

Removal of hud_radar.png also does not belong to the Armor system.

I just changed the icon texture while I made the armor icon texture. Doesn't belong to the armor system, yes. I should have changed the branch, will do it next time.

if dmginfo:GetDamage() <= 0 then return end

-- fallback for players who prefer the vanilla armor
if self.cv.armor_classic:GetBool() and ply:Armor() > 0 then

This comment has been minimized.

Copy link
@Alf21

Alf21 Sep 3, 2019

Member

pls give a look to this indent

return true
local item = items.GetStored(id)

if item and item.limited and self:HasEquipmentItem(id) then

This comment has been minimized.

Copy link
@Alf21

Alf21 Sep 3, 2019

Member

what if item is nil?

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 3, 2019

Author Member

you're right. It is not checked here. However self:AddEquipmentItem() checks if the item is nil

-- @realm shared
function plymeta:ArmorIsReinforced()
return self.armor_is_reinforced or false
end

This comment has been minimized.

Copy link
@Alf21

Alf21 Sep 3, 2019

Member

missing EOL

p:SetArmor(self.cv.armor_on_spawn:GetInt())
end
end
end

This comment has been minimized.

Copy link
@Alf21

Alf21 Sep 3, 2019

Member

missing EOL

if not IsValid(client) then return end

client.armor_max = net.ReadUInt(16)
end)

This comment has been minimized.

Copy link
@Alf21

Alf21 Sep 3, 2019

Member

missing EOL

@TimGoll TimGoll force-pushed the armor-system branch from b9d3a8c to 0491689 Sep 3, 2019

@saibotk
Copy link
Member

left a comment

Please move all unrelated changes to another PR etc, so that this is easier to review and the changes are separated.
Additionally max armor should be considered in all places, when adding armor / setting max armor, this should not be left as a purely visual variable.
The lang strings still need to be changed, when classic mode is on.

@@ -0,0 +1,59 @@
-- HANDLE ARMOR STATUS ICONS
-- init icons
hook.Add("Initialize", "ttt2_base_register_armor_status", function()

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 5, 2019

Member

Please do not use Hooks, when adding features in TTT2 as a base, since you can add this function to the Initialize function. Maybe call it ARMOR:Initialize() or so

self.cv.health_factor = self.cv.health_factor - 0.15
end

armor = armor - self.cv.armor_factor * damage

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 5, 2019

Member

This will still allow negative damage to increase the current armor.


-- before the karma is calculated, but after all other damage hooks / damage change is processed,
-- the armor system should come into place (GM functions are called last)
ARMOR:HandlePlayerTakeDamage(ent, infl, att, amount, dmginfo)

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 5, 2019

Member

This should not be excluded from the upper if, instead it would be good, to include it, since the checks are useful for the armor handling too and it would remove the duplicate if.

@@ -19,6 +19,12 @@ function ITEM:Equip(buyer)
end
end

function ITEM:Reset(buyer)
if SERVER then
buyer:DecreaseArmor(GetConVar("ttt_item_armor_value"):GetInt())

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 5, 2019

Member

That is not a valid solution, since it is possible to have many sources, where additional armor comes from, this would remove armor points, without considering the armor that is left, from this item. (eg. get 2 armor items +100, get shot -10, remove one -100 and you got less than you should

-- Sets the max armor to a specific value
-- @param number armor_max the new max armor to be set
-- @realm server
function plymeta:SetMaxArmor(armor_max)

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 5, 2019

Member

This will not change the current armor accordingly

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 5, 2019

Author Member

MaxArmor is only used for HUDs

-- @param number armor the new armor to be set
-- @realm server
function plymeta:SetArmor(armor)
self.armor = armor

This comment has been minimized.

Copy link
@saibotk

saibotk Sep 5, 2019

Member

This does not consider max armor

This comment has been minimized.

Copy link
@TimGoll

TimGoll Sep 5, 2019

Author Member

like I said previously: This system is build the same as the health system, therefore MaxArmor is only used for HUDs

TimGoll added 5 commits Sep 5, 2019
making jenkins happy
(and tobi!)
oof
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.