Skip to content

Conversation

@pledbrook
Copy link

Ability prerequisites that begin with "NOT_" are treated as mutually
exclusive abilities and are displayed as such in the ability preview.

This takes the pull request that Musashi sent to the original NPSBD mod
and applies it to the community version.

Implements #4.

{
PreviousAbilityTemplate = AbilityTemplateManager.FindAbilityTemplate(PrereqAbilityName);
if (PreviousAbilityTemplate != none && !Unit.HasSoldierAbility(PrereqAbilityName))
if (InStr(PrereqAbilityName, "NOT_") == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (InStr(PrereqAbilityName, "NOT_") == 0)
if (InStr(PrereqAbilityName, "NOT_", , true) == 0)

Plain InStr on coerced names is bad because names are supposed to be case insensitive. true as the fourth function argument fixes this by making InStr case insensitive. I realize the Highlander also doesn't do this; can you tack on the change to X2CommunityCore/X2WOTCCommunityHighlander#601 ?

Copy link
Author

Choose a reason for hiding this comment

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

@robojumper Is the Repl() below also case sensitive? Perhaps it would be better to use Mid() to remove the first 4 characters, since we've already checked it starts with "NOT_". I do wonder whether @Musashi1584 wanted to make this prefix case sensitive though.

@pledbrook pledbrook requested a review from Musashi1584 February 4, 2020 12:06
@Iridar Iridar force-pushed the mutually-exclusive-abilities branch from b3b2edd to 00b02c6 Compare May 16, 2021 16:16
Iridar51 added 2 commits May 16, 2021 19:17
@Iridar Iridar force-pushed the mutually-exclusive-abilities branch from 319b5f0 to daaf5f5 Compare May 16, 2021 16:18
@Iridar Iridar merged commit 778efcd into master May 16, 2021
@pledbrook pledbrook deleted the mutually-exclusive-abilities branch June 24, 2021 07:41
@Iridar Iridar added this to the 1.0 milestone Jul 26, 2021
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