-
Notifications
You must be signed in to change notification settings - Fork 16
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
SC2: Add Aberration Baneling Launch upgrade #322
SC2: Add Aberration Baneling Launch upgrade #322
Conversation
e474f50
to
1c956cd
Compare
Mods/ArchipelagoPlayer.SC2Mod/Base.SC2Data/GameData/RequirementNodeData.xml
Show resolved
Hide resolved
<LayoutButtons index="7" Face="AP_AberrationIncubateBanelings" Requirements="AP_HaveAberrationBanelingIncubation" Column="2"/> | ||
<LayoutButtons index="8" Face="AP_AberrationMonstrousResilience" Requirements="AP_HaveAberrationMonstrousResilience" Column="3"/> | ||
<LayoutButtons index="9" Face="AP_AberrationConstructRegeneration" Requirements="AP_HaveAberrationConstructRegeneration" Column="1"/> | ||
<LayoutButtons index="10"> |
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.
Looks like something went weird here with the buttons; I don't know why it's trying to include index="
in there, and that means AP_BurrowInfestedAbomincationDown
is essentially getting defined and then modified one line below.
The index="5"
line should be merged with the line above (I think just change ,0
to ,Execute
?), and then index=""
removed from all the following lines. index="10"
would be nice to have on a single line for style reasons, but that's optional.
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.
You shouldn't use indices here as they're for overriding stuff. This code creates a new unit with the most default parent, therefore there's no reason to override anything
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.
Should be fixed
<LayoutButtons index="7" Face="AP_AberrationIncubateBanelings" Requirements="AP_HaveAberrationBanelingIncubation" Column="2"/> | ||
<LayoutButtons index="8" Face="AP_AberrationMonstrousResilience" Requirements="AP_HaveAberrationMonstrousResilience" Column="3"/> | ||
<LayoutButtons index="9" Face="AP_AberrationConstructRegeneration" Requirements="AP_HaveAberrationConstructRegeneration" Column="1"/> | ||
<LayoutButtons index="10"> |
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.
You shouldn't use indices here as they're for overriding stuff. This code creates a new unit with the most default parent, therefore there's no reason to override anything
<CUnit id="AP_BanelingLaunchProjectile" parent="MISSILE"> | ||
<Race value="Zerg"/> | ||
<EditorCategories value="ObjectFamily:Campaign,ObjectType:Projectile"/> | ||
</CUnit> |
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.
Should Raven's PDD be able to shot down this? Spells often use MISSILE_INVULNERABLE
as a parent
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.
Swapped parent to MISSILE_INVULNERABLE
Mods/ArchipelagoPlayer.SC2Mod/Base.SC2Data/GameData/RequirementNodeData.xml
Outdated
Show resolved
Hide resolved
<CEffectDamage id="AP_BanelingLaunchExplodeDamageSmall" parent="DU_WEAP"> | ||
<AINotifyFlags index="HurtEnemy" value="1"/> | ||
<ImpactLocation Value="TargetUnit"/> | ||
<AttributeBonus index="Structure" value="30"/> | ||
<Kind value="Splash"/> | ||
<Amount value="30"/> | ||
<Death value="Disintegrate"/> | ||
<AreaArray Radius="2.2" Fraction="1"/> | ||
<ExcludeArray Value="Target"/> | ||
<SearchFilters value="Air;Player,Ally,Neutral,Missile,Item,Stasis,Dead,Hidden,Invulnerable"/> | ||
</CEffectDamage> |
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.
if it's an active ability, the damage should be spell, also should ignore armor (remove the parent)
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.
Removed the parent and the damage type line (should default to spell damage and 0 armor reduction now)
<EffectArray Operation="Set" Reference="Effect,AP_BanelingLaunchImpact,EffectArray[0]" Value="AP_BanelingLaunchSet"/> | ||
<EffectArray Operation="Set" Reference="Abil,AP_BanelingLaunchHangar,MaxCount" Value="3"/> | ||
<EffectArray Operation="Set" Reference="Button,AP_BanelingLaunch,Tooltip" Value="Button/Tooltip/AP_BanelingLaunchLevel2"/> | ||
<EffectArray Operation="Set" Reference="Abil,AP_BanelingLaunchHangar,InfoArray[Ammo1].Button.Requirements" Value="AP_BanelingLaunchArmLevel2"/> |
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.
Are requirements an upgradeable field? We generally don't use this style of requirement changing, instead having the requirement node tree check if the player has the upgrade. This might still work, but it would need eyes on it in testing.
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.
From my tests, it works, and made the requirement node tree a lot less painful to navigate, as I don't need multiple nested node comparisons.
It is used for example for the reaver magazine to change it from stopping at queue 10 instead of queue 5 after the upgrade.
<OperandArray value="AP_LTCountUnitAP_BanelingLaunchAmmoQueuedOrBetterAtUnit1"/> | ||
</CRequirementAnd> | ||
<CRequirementAnd id="AP_AndGTECountUpgradeAP_AberrationBanelingLaunchLevel2CompleteOnly1GTECountUnitAP_BanelingLaunchAmmoCompleteOnlyAtUnit3"> | ||
<Tooltip value="RequirementNode/Tooltip/AP_AP_AndGTECountUpgradeAP_AberrationBanelingLaunchLevel2CompleteOnly1GTECountUnitAP_BanelingLaunchAmmoCompleteOnlyAtUnit3"/> |
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.
This tooltip doesn't point to a key in GameStrings.txt, and the requirementNode doesn't look like it's used anywhere that would have it show up in the UI. This line should probably be removed.
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.
fixed
What is this fixing or adding?
This adds a progressive upgrade for the Aberration that allows it to throw Banelings at air units.
Level 1 allows the Aberration to generate 1 Baneling to throw at air units every 10 seconds for 30 AoE damage.
Level 2 allows the Aberration to store up to 3 Banelings to throw. Additionally, it can now consume Banelings to instantly get another charge. With the level 2 upgrade, launched Banelings are also affected by Baneling upgrades (damage to main target, increased radius, spawn splitterlings, heal).
Client PR
How was this tested?
Generated a local campaign with 1 zerg mission, added Aberrations and the Baneling Launch upgrade with server commands.