Skip to content

Conversation

@WildCard65
Copy link
Contributor

No description provided.

@WildCard65 WildCard65 mentioned this pull request Jul 31, 2014
4 tasks
@winstliu
Copy link
Contributor

#114...

Copy link
Member

Choose a reason for hiding this comment

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

It appears this file was unfortunately left out of the PR. You should also consider using static or something else as well if you constantly need data.

@psychonic
Copy link
Member

A RemoveWearable native was already added to the TF2 Tools extension in commit 4a400d9.

We tend to put features like this directly into natives so if something changes that would require a recompile (such as the C++ function prototype), consumer plugins would not all need to be recompiled.

An IsWearable function would be better made in an extension where it wouldn't require gamedata as you can just check for the existence of the DT_TFWearableItem datatable on the entity. This isn't actually required for most use though, at least for RemoveWearable, due to its implementation. Trying to remove a non-wearable entity with it is essentially a no-op.

As for EquipWearable, are there any uses for this that don't result in invisible items?

Also, changes similar to what you have in TF2_RemoveWeaponSlot2 are already being discussed on bug 6206.

The gamedata load of "ff2data" suggests that this is just a copy/paste of functionality from the popular FF2 plugin. If the work isn't yours, make sure to give credit where credit is due, at least noted in the PR., There are also numerous PrintToServer calls that don't really belong. If it's an error, an error should be thrown. If it's a warning important enough to be logged, it should be logged. Else, nothing.

@psychonic
Copy link
Member

Additionally, when writing SDKCall stocks like that, you should cache the call handles rather than doing the gamedata lookup and call preparation each time that the function is called.

@winstliu
Copy link
Contributor

Like psychonic said, #114 already seems to be fine for TF2_RemoveWearable, and there's a bug tracking TF2_RemoveWeaponSlot.

FF2 does not use this "ff2data", so I suspect that it's from his fork of FF2 (FF2-Alpha).

@WildCard65
Copy link
Contributor Author

Credit to friagram for writing this, the ff2data thing I thought I changed.
I'll add credit in morning as well as fix this up... unless a commit makes
this useless
On 2014-07-30 9:19 PM, "Nicholas Hastings" notifications@github.com wrote:

Additionally, when writing SDKCall stocks like that, you should cache the
call handles rather than doing the gamedata lookup and call preparation
each time that the function is called.


Reply to this email directly or view it on GitHub
#117 (comment)
.

Removed TF2_RemoveWearable stock as extension has it.
TODO:Convert IsWearable to tf2 extension.
Removed TF2_EquipWearable
@psychonic
Copy link
Member

I apologize for not being as clear as I could be.

I don't think that IsWearable is necessary. Is there a use case for it besides checking before calling RemoveWearable? As I noted before, RemoveWearable does not care if the entity is a wearable or not. If it is not, the function will just do nothing, rather than cause a crash or other bad behavior.

Additionally, changes to the TF2 stocks for removing weapons are already being discussed on bug 6206. Implementation should wait until we figure out how we want to handle it. You're welcome to join in the discussion there.

@WildCard65
Copy link
Contributor Author

I'll leave this PR open unless bug 6206 says otherwise.

@WildCard65
Copy link
Contributor Author

Closing this as the changes to tf2_stocks things are already implemented as well as physconic says that IsWearable isn't needed

@WildCard65 WildCard65 closed this Aug 3, 2014
@WildCard65 WildCard65 deleted the wearablefix branch August 3, 2014 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants