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.

Sign up for GitHub

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

Proper way to strip static attributes #40

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Proper way to strip static attributes #40

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a567fd
Proper way to strip static attributes
tsuza May 22, 2022
8fb0d58
Follow the codebase's code format rules
tsuza May 22, 2022
514fd9b
Add new keep_static_attrs value; Remove some hardcoded attributes; It…
tsuza May 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 47 additions & 4 deletions scripting/cwx/item_config.sp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,10 +305,53 @@ int EquipCustomItem(int client, const CustomItemDefinition item) {
SetEntProp(itemEntity, Prop_Send, "m_iAccountID", accountid);
}
}

// TODO: implement a version that nullifies runtime attributes to their defaults
SetEntProp(itemEntity, Prop_Send, "m_bOnlyIterateItemViewAttributes",
!item.bKeepStaticAttributes);

if(!item.bKeepStaticAttributes) {
Copy link
Owner

@nosoop nosoop May 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Might be time to refactor the config definition to support string values for keep_static_attrs? I think it's worth leaving the numeric options to the property.

Something like:

  • Numeric values = KeepStatic_False or KeepStatic_True; only toggle m_bOnlyIterateItemViewAttributes
  • "nullify" = KeepStatic_Nullify; run this code

Copy link
Owner

Choose a reason for hiding this comment

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

(Previous iterations claimed "2" was for nullifying; I don't have any issues with that approach either.)

Copy link
Author

Choose a reason for hiding this comment

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

I haven't experienced any issues by using this method to strip static attributes but sure. I'd go with 0 / 1 / nullify.

Copy link
Author

Choose a reason for hiding this comment

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

Addressed this in the newest commit.

TF2Attrib_RemoveAll(itemEntity);

char valueType[2];
char valueFormat[64];

int staticAttribs[16];
float staticAttribsValues[16];

TF2Attrib_GetStaticAttribs(item.defindex, staticAttribs, staticAttribsValues);
Copy link
Owner

Choose a reason for hiding this comment

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

TF2Econ_GetItemStaticAttributes instead? The game can technically store more than that.

Copy link
Author

Choose a reason for hiding this comment

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

That's the original approach I used since at the time TF2Attrib_GetStaticAttribs didn't work for some reason. I don't think it should cause any issues since I don't think there is a weapon that has over 16 attributes (?)

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that's probably fine. TF2Attrib_GetStaticAttribs returns the attribute count so it might be good to iterate up to that count as long as it's less than 16.

Copy link
Author

Choose a reason for hiding this comment

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

Addressed this in the newest commit.


for(int i = 0; i < 16; i++)
{
if(staticAttribs[i] == 0) {
continue;
}

// Probably overkill
if(staticAttribs[i] == 796 || staticAttribs[i] == 724 || staticAttribs[i] == 817 || staticAttribs[i] == 834
Copy link
Owner

Choose a reason for hiding this comment

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

If we can get a working implementation without hardcoded attributes, that'd be good. This kind of thing is brittle and not something I want to think about maintaining in the future.

I think the main ones would be paintkit_proto_def_index (834) and has team color paintkit (745); the other ones have empty description formats so I'd imagine they're not an issue.

Copy link
Owner

Choose a reason for hiding this comment

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

Scratch that; the attribute listing page lied to me. The other ones do have value_is_additive.

Copy link
Author

Choose a reason for hiding this comment

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

So should I remove them? Although hardcoding some attributes might not be the prettiest thing to do ( which is fair ), I did it to skip some unnecessary cycles and avoid some things breaking ( e.g. 817 inspect_viewmodel_offset, I don't know if nullifying would actually break some "QoL" aspects of the weapons so I put them just in case ).

Copy link
Owner

Choose a reason for hiding this comment

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

I'd say remove them, but at the same time having other things be potentially broken isn't great. This was also one of the things I wasn't satisfied with when I was brainstorming the thing myself.

Copy link
Author

Choose a reason for hiding this comment

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

That's fair. However, the amount of attributes is so small that it's negligible. It'd be nice to have an automatic way though.

Copy link
Author

Choose a reason for hiding this comment

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

Addressed this in the newest commit.

|| staticAttribs[i] == 745 || staticAttribs[i] == 731 || staticAttribs[i] == 746) {
continue;
}

// "stored_as_integer" is absent from the attribute schema if its type is "string".
// TF2ED_GetAttributeDefinitionString returns false if it can't find the given string.
if(!TF2Econ_GetAttributeDefinitionString(staticAttribs[i], "stored_as_integer", valueType, sizeof(valueType))) {
continue;
}

TF2Econ_GetAttributeDefinitionString(staticAttribs[i], "description_format", valueFormat, sizeof(valueFormat));

// Since we already know what we're working with and what we're looking for, we can manually handpick
// the most significative chars to check if they match. Eons faster than doing StrEqual or StrContains.

if(valueFormat[9] == 'a' && valueFormat[10] == 'd') { // value_is_additive & value_is_additive_percentage
Copy link
Owner

@nosoop nosoop May 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fairly sure this also breaks Flame Throwers — one of the frequent complaints (with CW3) is that after Jungle Inferno, not preserving attributes disabled the new FT params. Granted they'd still be broken with the current keep_static_attrs implementation.

It doesn't show up in my attribute listing page but they're all value_is_additive in items_game (oddly enough, IEconItems_440/GetSchemaOverview doesn't match here).

Copy link
Owner

Choose a reason for hiding this comment

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

(Previously thought about in #2.)

Copy link
Author

Choose a reason for hiding this comment

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

Fair, I thought that it could break some niche weapons that had attributes that affected the functionality of the weapon. In my personal case, I didn't pay too much attention to it as it'd just be a matter of setting the attribute back to what it has to be in the config file of the weapon.
That's tricky. Would there even be a proper way to do it without hardcoding the attributes?

Copy link
Owner

Choose a reason for hiding this comment

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

With FT specifically, one could do a lookup to check the static attributes on the base item. The other base items (excluding spellbook / grappling hook / MvM canteen) only have weapon_stattrak_module_scale and kill eater score type * attributes. The FT does have extinguish restores health by default as well, though.

Copy link
Author

Choose a reason for hiding this comment

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

There is not a proper way to retrieve the base item unfortunately ( without doing some ugly boilerplate, at least ). So honestly I'd just hardcode the attributes to skip since there is only a small amount anyway.

TF2Attrib_SetByDefIndex(itemEntity, staticAttribs[i], 0.0);
}
else if((valueFormat[9] == 'i' && valueFormat[18] == 'p')
nosoop marked this conversation as resolved.
Show resolved Hide resolved
|| (valueFormat[9] == 'p' && valueFormat[10] == 'e')) { // value_is_percentage & value_is_inverted_percentage
TF2Attrib_SetByDefIndex(itemEntity, staticAttribs[i], 1.0);
}
else if(valueFormat[9] == 'o' && valueFormat[10] == 'r') { // value_is_or
TF2Attrib_SetByDefIndex(itemEntity, staticAttribs[i], 0.0);
}
}
}

// apply game attributes
if (item.nativeAttributes) {
Expand Down