Consistency bodygroups -> skin property #1245

Merged
merged 1 commit into from Sep 27, 2016

Projects

None yet

2 participants

@FPtje
Contributor
FPtje commented Sep 25, 2016

Effects have a field ent.AttachedEntity. When you have an effect that allows its skin to be changed through the properties menu, the actual :SetSkin method of the property is called with the prop_effect. When you have an effect that allows its bodygroup to be changed from the properties menu, the actualy :SetBodyGroup method of the property is called not with ent , but with ent.AttachedEntity. This is inconsistent and causes FPP to bug out.

This PR fixes that inconsistency. I tested it and setting body groups still works. As for addon breakage, unless addons specifically hardcoded that shit's fucked for bodygroups, this shouldn't break anything.

@FPtje FPtje Consistency bodygroups -> skin property
Effects have a field `ent.AttachedEntity`. When you have an effect that allows its skin to be changed through the properties menu, the actual `:SetSkin` method of the property is called with the `prop_effect`. When you have an effect that allows its bodygroup to be changed from the properties menu, the actualy `:SetBodyGroup` method of the property is called **not** with `ent` , but with `ent.AttachedEntity`. This is inconsistent and causes FPP to bug out. 

This PR fixes that inconsistency. I tested it and setting body groups still works. As for addon breakage, unless addons specifically hardcoded that shit's fucked for bodygroups, this shouldn't break anything.
81c46cd
@robotboy655
Collaborator

What's up with the unnecessary changes on the top?

@FPtje
Contributor
FPtje commented Sep 26, 2016

The top change:

local target = IsValid( ent.AttachedEntity ) and ent.AttachedEntity or ent

is necessary. Leaving that change out will leave target undefined. What do you mean?

@FPtje
Contributor
FPtje commented Sep 26, 2016

I'll reply anyway. It's relevant:

  1. That's consistent with what skin does
  2. Filter expects to find an effect, not a prop_dynamic
  3. ent is needed in the :SetBodyGroup call

So yeah, both target and ent are needed.

@robotboy655
Collaborator

Changing it from ent to target is completely pointless?

@FPtje
Contributor
FPtje commented Sep 26, 2016
@robotboy655 robotboy655 merged commit 726c335 into garrynewman:master Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment