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

WIP: Csharp torque3d work #2365

Closed
wants to merge 5 commits into from

Conversation

lukaspj
Copy link
Contributor

@lukaspj lukaspj commented Aug 1, 2019

No description provided.

@dottools
Copy link

dottools commented Aug 1, 2019

Why are you changing AFX fields? They're not normal properties as for most of them each time they're changed it's pushing the field referenced datablock value to a dynamic array to queue up multiple identical elements of the same type. Hence why they're prefixed with add to indicate it's adding an additional element to the respective effect/action type.

@lukaspj
Copy link
Contributor Author

lukaspj commented Aug 1, 2019

I realize that there is a reason they are pre-fixed.
The issue is that there is already a method called "addEffect":

DefineEngineMethod( afxPhraseEffectData, addEffect, void, ( afxEffectBaseData* effectData ),,

addFieldV("addEffect", TYPEID< afxEffectBaseData >(), myOffset(dummy_fx_entry), &emptyValidator,

Which one of these do I get if I do %obj.addEffect ?
Personally I believe these fields are a weird sort of ugly magic and I don't agree with it. But if they have to exist, they should be named something else. I don't really care what, but it shouldn't overlap with the method.

@Azaezel
Copy link
Contributor

Azaezel commented Aug 1, 2019

based on a review of the docs and samples, would say keep the varname, change the method to push

@lukaspj
Copy link
Contributor Author

lukaspj commented Aug 3, 2019

Closed in favor of #2366
Because I changed branch.

@lukaspj lukaspj closed this Aug 3, 2019
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.

None yet

3 participants