Exposing detailed knife details #431

Falderebet · 2023-10-05T11:25:15Z

Why this PR is needed

As of now it is not possible to see what "type" of knife players have equipped as they are all maped to EqKnife. This PR is an attempt to make that possible.

What this PR adds

New EquipmentTypes for the different knives.
Refactored MapEquipment as it no longer needs to handle "edge" cases that knives where previously.

markus-wa · 2023-10-09T09:03:11Z

pkg/demoinfocs/common/equipment.go

-		wep = eqNameToWeapon[eqName]
-	}
+	// If the eqName isn't known it will be EqUnknown as that is the default value for EquipmentType
+	wep := eqNameToWeapon[eqName]


hmm, this doesn't really scale well for when they add new knifes - we would need to update the parser every time. and worse, our consumers would need to upgrade, otherwise they will get EqUnknown for the new knife, which would be really hard to debug.

I suggest that we instead expose the raw eqName somewhere upstream in the call chain

That is a good point, I will take a look at it!

**Edit - I found the attribute "m_sWeaponName". Is that what you (@markus-wa) meant regarding exposing the raw eqName?

If so disregard the rest of the comment.

***Edit2 - I digged a bit deeper regarding the "m_sWeaponName" property, and it seems like it is not possible to get any relevant data from it. I have not found any other properties that holds weapon name data. Let me know if you know of any.

I don't think there is a good way to handle valve adding new knives. However, I think there are some options:

Exposing the weapon "code"/itemindex - eg. we give the consumers the possibility to see what code the EqUnkown has.

Pros:

This technically means that consumers can still handle new knives even before the parser is updated.

Cons:

It will still to a degree be hard for consumers to debug, as they only get a weapon code and nothing "human readable".

Parser will still need to be updated to handle new knives with EqTypes and so forth.

Exposing a more raw weapon string.

Pros:

Makes it more human readable for consumers to debug.

Cons:

The parser needs to be updated before consumers can add new knives.

After writing this out I find myself gravitating more towards option 1. As the raw weapon string is not something coming from the engine but something we attach from the item index.

also why not keeping strings.Contains(eqName, "knife") as a fallback when wep is unknown?
i think it's better to have weapon_knife instead of "Unknown" when we know it's a knife

also why not keeping strings.Contains(eqName, "knife") as a fallback when wep is unknown? i think it's better to have weapon_knife instead of "Unknown" when we know it's a knife

My thought was that the parser defines what a knife "is". Therefore it would be an error in the parser if the EqName did not map to an EqType.

I can add it back, I guess it is nice to have if new knives are added and the eqname/eqtype map is not updated.

Actually - I don't think we can add new EqKnifeXYZ types anyway as it's not backwards compatible and would lead to confusion (e.g. if the user wants to check if a kill was a knife kill, using EqKnife is more straight forward than checking all cases.

So we should expose this as additional data, not as replacement.

So we'll need another way of solving this - not yet sure how exactly

What about the idea of adding a field to the equipment struct with additional detailes about the weapon/equipment?

Let me know if there is something I can do. :)

That sounds good! Maybe something similar to what we already have with Equipment.OriginalString but for this new string?

markus-wa

as discussed in comments

codecov · 2023-10-23T07:26:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8bf8a71) 80.38% compared to head (3256112) 80.53%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
+ Coverage   80.38%   80.53%   +0.14%     
==========================================
  Files          47       47              
  Lines        6257     6304      +47     
==========================================
+ Hits         5030     5077      +47     
  Misses       1018     1018              
  Partials      209      209

Files	Coverage Δ
pkg/demoinfocs/common/equipment.go	`91.66% <100.00%> (+1.66%)`	⬆️
pkg/demoinfocs/datatables.go	`92.10% <100.00%> (+0.06%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…rebet/demoinfocs-golang into exposing-detailed-wep-data

markus-wa · 2023-10-26T10:59:28Z

pkg/demoinfocs/common/equipment.go

+	EqKnifeOutdoor             EquipmentType = 437
+	EqKnifeStiletto            EquipmentType = 438
+	EqKnifeWidowmaker          EquipmentType = 439
+	EqKnifeSkeleton            EquipmentType = 440


the RawEquipment part looks good now - but this part will need to be removed as it's not backwards compatible as mentioned in the other thread.

So all we should do for now is expose RawEquipment.

Maybe we can have a separate type KnifeType and constants KnifeTypeXYZ with a map KnifeTypes map[string]KnifeType which includes all known knife types to still provide some kind of mapping.

The good thing about this is that if a new knife comes out the users can just add it to the map themselves,. as the map will be mutable.

I think this would be the best middleground

markus-wa · 2023-10-26T10:59:48Z

pkg/demoinfocs/common/equipment.go

+	eqNameToWeapon["knife_outdoor"] = EqKnifeOutdoor
+	eqNameToWeapon["knife_stiletto"] = EqKnifeStiletto
+	eqNameToWeapon["knife_widowmaker"] = EqKnifeWidowmaker
+	eqNameToWeapon["knife_skeleton"] = EqKnifeSkeleton


same here, would need to be moved to the new map I wrote about

markus-wa · 2023-10-26T11:00:20Z

pkg/demoinfocs/common/equipment.go

+	eqElementToName[EqKnifeOutdoor] = "Outdoor Knife"
+	eqElementToName[EqKnifeStiletto] = "Stiletto Knife"
+	eqElementToName[EqKnifeWidowmaker] = "Widowmaker Knife"
+	eqElementToName[EqKnifeSkeleton] = "Skeleton Knife"


this would have to be in a new map knifeTypeToName - don't forget to implement the String() method on KnifeType

markus-wa · 2023-10-26T11:01:15Z

pkg/demoinfocs/common/equipment.go

+	521: EqKnifeOutdoor,             // weapon_knife_outdoor
+	522: EqKnifeStiletto,            // weapon_knife_stiletto
+	523: EqKnifeWidowmaker,          // weapon_knife_widowmaker
+	525: EqKnifeSkeleton,            // weapon_knife_skeleton


these would need to be put in a new map as well KnifeTypeIndexMapping

markus-wa · 2023-10-26T11:02:46Z

thanks for the changes @Falderebet - sorry this is taking a while, but I think it's important to get this right to prevent issues in the future - thanks for your patience here!

Falderebet added 3 commits October 5, 2023 12:57

added knife types

5c94794

added test coverage for mapping of knives.

94767d6

linting

f796928

markus-wa reviewed Oct 9, 2023

View reviewed changes

Falderebet requested a review from markus-wa October 10, 2023 10:39

markus-wa self-assigned this Oct 16, 2023

markus-wa requested changes Oct 20, 2023

View reviewed changes

markus-wa added the needs: changes (contributor) label Oct 20, 2023

Merge branch 'markus-wa:master' into exposing-detailed-wep-data

25f3d76

Falderebet and others added 5 commits October 23, 2023 14:56

Merge branch 'master' into exposing-detailed-wep-data

15daf5e

added RawEquipment to equipment struct

5fb46b3

added handling of rawequipment when binding weapon

2bd6010

Merge branch 'exposing-detailed-wep-data' of https://github.com/Falde…

1a4fcdb

…rebet/demoinfocs-golang into exposing-detailed-wep-data

Merge branch 'markus-wa:master' into exposing-detailed-wep-data

3505ba3

Falderebet requested a review from markus-wa October 23, 2023 14:06

Falderebet added 2 commits October 23, 2023 16:13

fixed eq tests

85fcd76

Merge branch 'exposing-detailed-wep-data' of https://github.com/Falde…

d71becb

…rebet/demoinfocs-golang into exposing-detailed-wep-data

markus-wa reviewed Oct 26, 2023

View reviewed changes

Merge branch 'markus-wa:master' into exposing-detailed-wep-data

3256112

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposing detailed knife details #431

Exposing detailed knife details #431

Falderebet commented Oct 5, 2023

markus-wa Oct 9, 2023

Falderebet Oct 9, 2023

Falderebet Oct 9, 2023 •

edited

akiver Oct 9, 2023

Falderebet Oct 9, 2023

markus-wa Oct 12, 2023 •

edited

Falderebet Oct 13, 2023

markus-wa Oct 16, 2023 •

edited

markus-wa left a comment

codecov bot commented Oct 23, 2023 •

edited

markus-wa Oct 26, 2023

markus-wa Oct 26, 2023

markus-wa Oct 26, 2023

markus-wa Oct 26, 2023

markus-wa commented Oct 26, 2023

Exposing detailed knife details #431

Are you sure you want to change the base?

Exposing detailed knife details #431

Conversation

Falderebet commented Oct 5, 2023

Why this PR is needed

What this PR adds

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Falderebet Oct 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus-wa Oct 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus-wa Oct 16, 2023 • edited

Choose a reason for hiding this comment

markus-wa left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 23, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus-wa commented Oct 26, 2023

Falderebet Oct 9, 2023 •

edited

markus-wa Oct 12, 2023 •

edited

markus-wa Oct 16, 2023 •

edited

codecov bot commented Oct 23, 2023 •

edited