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

Fire weapon when kill #1440

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Coronia
Copy link
Contributor

@Coronia Coronia commented Dec 2, 2024

  • KillWeapon will be fired at the target TechnoType's location once it's been killed by this Warhead.
    • KillWeapon.AffectTargets is used to filter which types of targets (TechnoTypes) are considered valid for KillWeapon. Only none, all, aircraft, buildings, infantry and units are valid values.
    • KillWeapon.AffectHouses is used to filter which houses targets can belong to be considered valid for KillWeapon.
    • KillWeapon.AffectTypes can be used to list specific TechnoTypes to be considered as valid targets for KillWeapon. If any valid TechnoTypes are listed, then only matching objects will be targeted.
    • KillWeapon.IgnoreTypes can be used to list specific TechnoTypes to be never considered as valid targets for KillWeapon.
  • Ìf a TechnoType has SuppressKillWeapons set to true, it will not trigger KillWeapon upon being killed. SuppressKillWeapons.Types can be used to list WeaponTypes affected by this, if none are listed all WeaponTypes are affected.

In rulesmd.ini:

[SOMEWARHEAD]                   ; Warhead
KillWeapon=                     ; WeaponType
KillWeapon.AffectTargets=all    ; list of Affected Target Enumeration (none|aircraft|buildings|infantry|units|all)
KillWeapon.AffectHouses=all     ; list of Affected House Enumeration (none|owner/self|allies/ally|team|enemies/enemy|all)
KillWeapon.AffectTypes=         ; list of TechnoTypes
KillWeapon.IgnoreTypes=         ; list of TechnoTypes

[SOMETECHNO]                    ; TechnoType
SuppressKillWeapons=false       ; boolean
SuppressKillWeapons.Types=      ; list of WeaponTypes

Copy link

github-actions bot commented Dec 2, 2024

Nightly build for this pull request:

This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build.

Copy link
Contributor

@Starkku Starkku left a comment

Choose a reason for hiding this comment

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

Some stuff that probably needs to be checked but for most part it is the usual clean and basic stuff. Although idk what the intention or intended use cases here are so I can't comment much on the logic itself, just code / implementation.

Comment on lines +209 to +219
if (!pWHExt->KillWeapon || pTypeExt->SuppressKillWeapons || !EnumFunctions::CanTargetHouse(pWHExt->KillWeapon_AffectHouses, pSource->Owner, pThis->Owner))
return;

if (pWHExt->KillWeapon_AffectTypes.size() > 0 && !pWHExt->KillWeapon_AffectTypes.Contains(pType))
return;

if (pWHExt->KillWeapon_IgnoreTypes.size() > 0 && pWHExt->KillWeapon_IgnoreTypes.Contains(pType))
return;

if (pTypeExt->SuppressKillWeapons_Types.size() > 0 && pTypeExt->SuppressKillWeapons_Types.Contains(pWHExt->KillWeapon))
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering this is copypasted from revenge weapons or whatever, could consider moving the code to a function that returns a bool and takes the appropriate vectors (pass by reference ig) as params etc.

src/Ext/Techno/WeaponHelpers.cpp Outdated Show resolved Hide resolved
@Metadorius
Copy link
Member

I am once again asking for introduction of either EventHandlerType or a template to do uniform event handlers. Then instead of reinventing the same stuff with different features (like here you have Affects*) you could generalize that and have it available everywhere in a uniform fashion.

@Coronia
Copy link
Contributor Author

Coronia commented Dec 3, 2024

I'm still not sure what pattern should be used to group these conditions together. The current condition lines take reference from both RevengeWeapon and DetonateOnAllMapObjects, but these 3 things have some slight difference that I need to think of how to sort them out if making them into universal logic.

Besides, we're not just talking about something for my own pull requests, but also a shared template for the future. Think we'd better agree on a pattern that's acceptable by most first, and better makes a demo for that. I could help adding functions into it if the template is defined.

@Metadorius
Copy link
Member

Metadorius commented Dec 3, 2024

I'm still not sure what pattern should be used to group these conditions together. The current condition lines take reference from both RevengeWeapon and DetonateOnAllMapObjects, but these 3 things have some slight difference that I need to think of how to sort them out if making them into universal logic.

I think we're talking about different sides of the same problem. What you're talking about is the condition part, whereas I am talking about the sequence part.

I'll create a discussion/issue where we should document that then and where everyone could take part.

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.

3 participants