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

[Minor] Attach effect when weapon fire #1482

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

Conversation

CrimRecya
Copy link
Contributor

@CrimRecya CrimRecya commented Dec 29, 2024

AttachEffect will directly act on the target at the instant after firing.

[SOMEWEAPON]                                       ; WeaponType
AttachEffect.AttachTypes=                          ; List of AttachEffectTypes
AttachEffect.CumulativeRefreshAll=false            ; boolean
AttachEffect.CumulativeRefreshAll.OnAttach=false   ; boolean
AttachEffect.CumulativeRefreshSameSourceOnly=true  ; boolean
AttachEffect.RemoveTypes=                          ; List of AttachEffectTypes
AttachEffect.RemoveGroups=                         ; comma-separated list of strings (group IDs)
AttachEffect.CumulativeRemoveMinCounts=            ; integer - minimum required instance count (comma-separated) for cumulative types in order from first to last.
AttachEffect.CumulativeRemoveMaxCounts=            ; integer - maximum removed instance count (comma-separated) for cumulative types in order from first to last.
AttachEffect.DurationOverrides=                    ; integer - duration overrides (comma-separated) for AttachTypes in order from first to last.

More general expansion for #1472 .

@github-actions github-actions bot added the Minor Documentation is not required label Dec 29, 2024
Copy link

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.

@Coronia
Copy link
Contributor

Coronia commented Jan 15, 2025

I would say we should still keep AE for warhead only, especially when we can use ExtraWarheads and such to provide different effects for different technos in one shot. Adding a connection between weapon and AE would make the system overcomplicated, while not gaining much

@CrimRecya
Copy link
Contributor Author

I would say we should still keep AE for warhead only, especially when we can use ExtraWarheads and such to provide different effects for different technos in one shot. Adding a connection between weapon and AE would make the system overcomplicated, while not gaining much

The biggest advantage of weapon AE over warhead AE is that it is completed in an instant. It can produce different effects with the order of unit update, and the warhead AE will not take effect until all bullets are updated and detonated. Sometimes, it is too late.

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.

It is potentially niche but the code is not as complex as it could be since there's very little bespoke code (parsing is handled by AEAttachInfoTypeClass which can directly be passed to the attach/detach functions). Whether or not there's a performance overhead that outweighs potential benefits is hard to quantify, though.

There's one small thing that should be changed at the very least.

src/Ext/Techno/Hooks.Firing.cpp Show resolved Hide resolved
@Speederovsky
Copy link

Why make this when you could design something like an InstantWarhead detonation that skips all these checks too and gives us more flexibility to what the instantly applied effects actually are? Not just AE, and you skip having to redo all nuAE tags on WeaponTypes.

@CrimRecya
Copy link
Contributor Author

Why make this when you could design something like an InstantWarhead detonation that skips all these checks too and gives us more flexibility to what the instantly applied effects actually are? Not just AE, and you skip having to redo all nuAE tags on WeaponTypes.

Your idea touched me. If killing some units in the process of traversing all units will not cause problems in the iterator due to DTOR or quantity reduction (maybe, not sure), it is worth trying.

@CrimRecya
Copy link
Contributor Author

After evaluation, InstantWarhead is considered to be unhelpful to the content of this pr. The same warhead is required if AE needs to be used to search target by the weapon. That is, InstantWarhead should be used as the Boolean value of a projectile with Inviso=true, so that the projectile will detonate immediately after it is set in position, instead of waiting for a uniform update time.

@Metadorius
Copy link
Member

I didn't quite get why is InstantWarhead unsuitable, can you elaborate?

@CrimRecya
Copy link
Contributor Author

CrimRecya commented Jan 20, 2025

I didn't quite get why is InstantWarhead unsuitable, can you elaborate?

As discussed with Starkku before, if the weapon needs to use AE to target (such as IgnoreFromSameSource), the warhead of the weapon must be the same. If it wants to replace the requirement that PR can solve, it should move to the target position immediately after the projectile is generated, and then detonate immediately (to ensure that the Warhead is the same). If we use a new logic similar to ExtraWarhead to carry a new warhead, we can not achieve the purpose of this PR.
In short, it should be possible, but it is different from the goal of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Minor Documentation is not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants