-
Notifications
You must be signed in to change notification settings - Fork 74
Custom ability target styles v2 #918
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
Custom ability target styles v2 #918
Conversation
|
I personally have no use for this, but #762 in limbo bothered me, cc @Xymanek. #762 says LWotC needs this in some form, cc @pledbrook. |
X2WOTCCommunityHighlander/Src/XComGame/Classes/XComGameState_Ability_CH.uc
Outdated
Show resolved
Hide resolved
| if (AvailableCode != 'AA_Success') | ||
| return AvailableCode; | ||
|
|
||
| Targets.Sort(SortAvailableTargets); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use CH_MAKE_QUICKSORT from #809 since this is so performance critical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs profiling. My intuition is that anything but .Sort() will worsen performance. How many available targets do we expect? Note that #809 currently already cuts off at 15 elements to use InsertionSort.
|
I take it this has nothing to do with targeting methods? |
|
LWOTC has a |
For cursor-targeted abilities, the multi-target style builds the multi targets for the cursor target created by the targeting method. For everything else, the target style builds the initial target list, the multi-target style builds the multi-targets for every target, and the targeting method only selects a particular target. |
|
|
|
Wait, so it's all done and ready? Just needs a review? Imma get into that soon as I have time. |
pledbrook
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it looks fine to me. I think it just requires some testing to ensure it doesn't break anything. Could use it for the next LWOTC release so that it does get some level of playtesting.
Iridar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this a while ago, but forgot to report in. It works perfectly fine and is sufficient to do what I wanted to do, which is to make a version of Faceoff that can target variable number of targets based on pistol's current ammo.
Fixes #763, resubmission of #762.