Burst projectile retargeting #1073
Conversation
|
Regarding how affects the Omnifire tag. |
|
Appears to work fine (even tried it for Trajectory=Straight projectiles), except for a case where a jumpjet without a turret and with OmniFire=no retargeted its burst fire into a landed AircraftType that was on a straight line behind. Same thing happened multuple times. The jumpjet also had a secondary weapon that it switches to after running out of ammo, and that one has OmniFire=yes but it doesn't retarget. |
What kind of weapons were used in the jumpjet case? I specified they should be of the type missile (but I noticed that invisible bullets works too). |
|
I know you said that it was designed for regular missiles, but it also appears to works fine for Trajectory=Straight (also tried with Arcing=yes, and it doesn't work there at all). |
Starkku
force-pushed
the
develop
branch
2 times, most recently
from
aae92fd to
a32076e
Compare
Now isn't required to have Burst > 1 in the weapon.
|
I updated with the rewrite of the feature. Now the tag is called "RandomTarget" (docs updated and the first post has been updated) |
|
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. |
|
I found a potential issue with spawners on the Sept 22nd build, but can't test it if it still happens later because the newer build didn't build properly. |
WalkthroughThe recent updates introduce a new targeting feature for projectiles, allowing them to randomly select targets within range. This includes the ability to retarget each projectile burst and enhancements to AI logic for target selection. The changes span across various classes, focusing on extending functionality for Techno, WeaponType, and Script classes, with a particular emphasis on random target handling and unit availability checks. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (11)
- CREDITS.md (1 hunks)
- YRpp (1 hunks)
- docs/New-or-Enhanced-Logics.md (1 hunks)
- docs/Whats-New.md (1 hunks)
- src/Ext/Techno/Body.Update.cpp (2 hunks)
- src/Ext/Techno/Body.cpp (3 hunks)
- src/Ext/Techno/Body.h (5 hunks)
- src/Ext/Techno/Hooks.Firing.cpp (2 hunks)
- src/Ext/Techno/Hooks.cpp (2 hunks)
- src/Ext/WeaponType/Body.cpp (2 hunks)
- src/Ext/WeaponType/Body.h (2 hunks)
Files skipped from review due to trivial changes (1)
- src/Ext/WeaponType/Body.cpp
Additional comments: 23
CREDITS.md (1)
- 128-131: The contribution of
Burst.Retargetfor projectile retargeting is correctly credited.YRpp (1)
- 1-1: The hunk provided does not contain any code to review. Please provide the relevant code hunks for review.
docs/Whats-New.md (1)
- 351-351: The changes to the documentation are clear and concise, providing a good summary of the new feature.
src/Ext/WeaponType/Body.h (2)
39-43: The new properties
RandomTargetandRandomTarget_Spawners_MultipleTargetsare added to theExtDataclass. Ensure that these properties are used correctly in the rest of the codebase.65-66: The new properties are initialized in the constructor of
ExtData. The initialization seems correct.src/Ext/Techno/Hooks.cpp (2)
5-9: New include statements for "Ext/BuildingType/Body.h" and "Ext/Script/Body.h" have been added. Ensure that these files exist and are accessible from this location.
27-33: The
RefreshRandomTargetsfunction is called within theTechnoClass_AIfunction. This suggests that the AI logic has been updated to handle the refreshing of random targets. Ensure that this function is implemented correctly and that it doesn't introduce any performance issues.src/Ext/Techno/Body.h (6)
9-9: The inclusion of
Utilities/EnumFunctions.his new. Ensure that this header file is available and that it contains the necessary functions or definitions used in this file.42-43: New member variables
OriginalTargetandCurrentRandomTargetare added to theExtDatastruct. Ensure that these variables are used appropriately in the code and that their values are properly managed.67-68: The constructor of
ExtDatais updated to initialize the new member variablesOriginalTargetandCurrentRandomTargettonullptr. This is a good practice to avoid undefined behavior.82-82: The new member function
RefreshRandomTargetsis added. Ensure that this function is implemented correctly and that it is called at appropriate places in the code.89-90: The
InvalidatePointerfunction is updated to handle the new member variablesOriginalTargetandCurrentRandomTarget. This is a good practice to ensure that these pointers are invalidated when the objects they point to are removed.154-155: The new static member functions
UpdateRandomTargetandGetRandomTargetare added. Ensure that these functions are implemented correctly and that they are called at appropriate places in the code.src/Ext/Techno/Hooks.Firing.cpp (9)
876-882: Ensure that the
UpdateRandomTargetfunction handles null pointers and other edge cases correctly.885-891: Same as above, ensure that the
UpdateRandomTargetfunction handles null pointers and other edge cases correctly.894-912: This hook seems to be changing the target of the
TechnoClassobject. Ensure that this change is intended and that it doesn't interfere with other parts of the code that might be using theTargetproperty.914-941: This hook seems to be iterating over
SpawnedNodesand updating theCurrentRandomTargetfor each node. Ensure that this operation is thread-safe ifSpawnedNodescan be accessed or modified from multiple threads.943-962: This hook seems to be updating the
CurrentRandomTargetfor aAircraftClassobject. Ensure that this operation is thread-safe ifCurrentRandomTargetcan be accessed or modified from multiple threads.964-983: Same as above, ensure that the operation is thread-safe if
CurrentRandomTargetcan be accessed or modified from multiple threads.985-1008: This hook seems to be iterating over
CurrentObjectsand updating theCurrentRandomTargetandOriginalTargetfor each object. Ensure that this operation is thread-safe ifCurrentObjects,CurrentRandomTarget, orOriginalTargetcan be accessed or modified from multiple threads.1010-1032: This hook seems to be updating the
CurrentRandomTargetandOriginalTargetfor aFootClassobject. Ensure that this operation is thread-safe ifCurrentRandomTargetorOriginalTargetcan be accessed or modified from multiple threads.1034-1052: This hook seems to be updating the
Targetfor aBulletClassobject. Ensure that this operation is thread-safe ifTargetcan be accessed or modified from multiple threads.src/Ext/Techno/Body.cpp (1)
- 868-873: The
Serializefunction has been updated to include theOriginalTargetandCurrentRandomTargetvariables. Ensure that the deserialization function is also updated to include these variables to maintain consistency.
src/Ext/Techno/Body.cpp
Outdated
| TechnoClass* TechnoExt::GetRandomTarget(TechnoClass* pThis) | ||
| { | ||
| TechnoClass* selection = nullptr; | ||
|
|
||
| if (!pThis && !pThis->Target) | ||
| return selection; | ||
|
|
||
| int weaponIndex = pThis->SelectWeapon(pThis->Target); | ||
| auto pWeapon = pThis->GetWeapon(weaponIndex)->WeaponType; | ||
| if (!pWeapon) | ||
| return selection; | ||
|
|
||
| const auto pWeaponExt = WeaponTypeExt::ExtMap.Find(pWeapon); | ||
| if (!pWeaponExt || pWeaponExt->RandomTarget <= 0.0) | ||
| return selection; | ||
|
|
||
| const auto pExt = TechnoExt::ExtMap.Find(pThis); | ||
| if (!pExt) | ||
| return selection; | ||
|
|
||
| int retargetProbability = std::min((int)round(pWeaponExt->RandomTarget * 100), 100); | ||
| int dice = ScenarioClass::Instance->Random.RandomRanged(1, 100); | ||
|
|
||
| if (retargetProbability < dice) | ||
| return selection; | ||
|
|
||
| auto pThisType = pThis->GetTechnoType(); | ||
| int minimumRange = pWeapon->MinimumRange; | ||
| int range = pWeapon->Range; | ||
| int airRange = pWeapon->Range + pThisType->AirRangeBonus; | ||
| bool omniFire = pWeapon->OmniFire; | ||
| std::vector<TechnoClass*> candidates; | ||
| auto originalTarget = abstract_cast<TechnoClass*>(!pExt->OriginalTarget ? pThis->Target : pExt->OriginalTarget); | ||
| bool friendlyFire = pThis->Owner->IsAlliedWith(originalTarget); | ||
|
|
||
| // Looking for all valid targeting candidates | ||
| for (auto pCandidate : *TechnoClass::Array) | ||
| { | ||
| if (pCandidate == pThis | ||
| || !ScriptExt::IsUnitAvailable(pCandidate, true) | ||
| || pThisType->Immune | ||
| || !EnumFunctions::IsTechnoEligible(pCandidate, pWeaponExt->CanTarget, true) | ||
| || (!pWeapon->Projectile->AA && pCandidate->IsInAir()) | ||
| || (!pWeapon->Projectile->AG && !pCandidate->IsInAir()) | ||
| || (!friendlyFire && (pThis->Owner->IsAlliedWith(pCandidate) || ScriptExt::IsUnitMindControlledFriendly(pThis->Owner, pCandidate))) | ||
| || pCandidate->TemporalTargetingMe | ||
| || pCandidate->BeingWarpedOut | ||
| || (pCandidate->GetTechnoType()->Underwater && pCandidate->GetTechnoType()->NavalTargeting == NavalTargetingType::Underwater_Never) | ||
| || (pCandidate->GetTechnoType()->Naval && pCandidate->GetTechnoType()->NavalTargeting == NavalTargetingType::Naval_None) | ||
| || (pCandidate->CloakState == CloakState::Cloaked && !pThisType->Naval) | ||
| || (pCandidate->InWhichLayer() == Layer::Underground)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| int distanceFromAttacker = pThis->DistanceFrom(pCandidate); | ||
| if (distanceFromAttacker < minimumRange) | ||
| continue; | ||
|
|
||
| if (omniFire) | ||
| { | ||
| if (pCandidate->IsInAir()) | ||
| range = airRange; | ||
|
|
||
| if (distanceFromAttacker <= range) | ||
| candidates.push_back(pCandidate); | ||
| } | ||
| else | ||
| { | ||
| int distanceFromOriginalTargetToCandidate = pCandidate->DistanceFrom(pThis->Target); | ||
| int distanceFromOriginalTarget = pThis->DistanceFrom(pThis->Target); | ||
|
|
||
| if (pCandidate->IsInAir()) | ||
| range = airRange; | ||
|
|
||
| if (distanceFromAttacker <= range && distanceFromOriginalTargetToCandidate <= distanceFromOriginalTarget) |
There was a problem hiding this comment.
The GetRandomTarget function is also quite complex. Similar to the UpdateRandomTarget function, consider breaking it down into smaller functions for better readability and maintainability. For example, the part where it checks if the candidate is valid could be a separate function. This would make the code easier to understand and maintain.
There was a problem hiding this comment.
I am also not sure what does this function do and why is it in ScriptExt: ScriptExt::IsUnitMindControlledFriendly(pThis->Owner, pCandidate)
There was a problem hiding this comment.
@Metadorius The function ScriptExt::IsUnitMindControlledFriendly(pThis->Owner, pCandidate) is likely used to check if a unit (pCandidate) is mind-controlled and is friendly to the owner (pThis->Owner).
The reason it's in ScriptExt might be because it's a utility function that can be used in various scripts, not just in this context. It's common to place such utility functions in a separate class or namespace (like ScriptExt) to avoid code duplication and improve code organization.
However, without more context or access to the ScriptExt class, this is just an educated guess. You might want to check the implementation of ScriptExt::IsUnitMindControlledFriendly for a more accurate understanding.
There was a problem hiding this comment.
Function ScriptExt::IsUnitMindControlledFriendly() was originally placed in ScriptExt because that is the only place where it was called. If it is now being called elsewhere it should likely be relocated to TechnoExt since that would be more appropriate for function that checks TechnoClass state. Same for ScriptExt::IsUnitAvailable().
There was a problem hiding this comment.
Looks like humans are talking. I'll be quiet for now. If you need me again, just tag @coderabbitai in a new comment.
There was a problem hiding this comment.
We usually place static extension functions as if they were the non-static functions of the class that is being extended (think of C# extension methods), so if it accepts the TechnoClass, it should be in TechnoExt. I am not sure if the description you gave is accurate, I would say that it needs a clarification from @FS-21.
There was a problem hiding this comment.
Do you mean ScriptExt::IsUnitMindControlledFriendly()? I don't touch what other people do, like that ScriptExt::IsUnitAvailable().
I'll move them to TechnoExt then.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (9)
- src/Ext/Script/Body.cpp (5 hunks)
- src/Ext/Script/Body.h (1 hunks)
- src/Ext/Script/Mission.Attack.cpp (7 hunks)
- src/Ext/Script/Mission.Move.cpp (4 hunks)
- src/Ext/Techno/Body.Update.cpp (2 hunks)
- src/Ext/Techno/Body.cpp (3 hunks)
- src/Ext/Techno/Body.h (6 hunks)
- src/Ext/Techno/Hooks.Firing.cpp (2 hunks)
- src/Ext/Techno/Hooks.cpp (2 hunks)
Files skipped from review due to trivial changes (3)
- src/Ext/Script/Body.cpp
- src/Ext/Script/Body.h
- src/Ext/Script/Mission.Move.cpp
Additional comments: 15
src/Ext/Techno/Hooks.Firing.cpp (1)
- 872-1052:
Ensure that the
TechnoExt::IsUnitAvailablefunction correctly checks for unit availability, as it's used to determine if the current random target is still valid. This is crucial for the correct functioning of theBurst.Retargetfeature.src/Ext/Techno/Hooks.cpp (1)
- 27-33: The addition of
pExt->UpdateRandomTargets();within theTechnoClass_AIfunction is correct and aligns with the summary provided. However, ensure that theUpdateRandomTargetsmethod is thread-safe ifTechnoClass_AIcan be called from multiple threads, as this could lead to race conditions.src/Ext/Techno/Body.Update.cpp (2)
6-12:
No action needed regarding the headers as they have been verified to be in use.734-767:
The implementation ofUpdateRandomTargetsseems correct and aligns with the feature's intent. However, ensure that theIsUnitAvailablefunction correctly handles all edge cases, especially since it has been moved fromScriptExttoTechnoExt.src/Ext/Techno/Body.h (5)
6-12: The inclusion of multiple
Utilitiesheaders seems redundant, asMacro.his included twice (lines 6 and 8). Verify if this is intentional or if one of the includes can be removed to avoid potential confusion or conflicts.39-43: The addition of new member variables
OriginalTargetandCurrentRandomTargetis consistent with the summary provided. Ensure that these new variables are properly initialized, used, and maintained throughout the codebase.82-82: The
UpdateRandomTargetsfunction is added as per the summary. However, ensure that this function is implemented and called appropriately within the codebase to maintain the new feature's functionality.126-130: The
IsUnitAvailablefunction has been moved fromScriptExttoTechnoExtas mentioned in the summary. Verify that all references to this function have been updated to reflect its new location.155-156: The addition of
UpdateRandomTargetandFindRandomTargetfunctions aligns with the summary. Ensure that these functions are implemented and that their logic correctly supports the new retargeting feature.src/Ext/Script/Mission.Attack.cpp (6)
115-120: The check for
TechnoExt::IsUnitAvailable(pFoot, true)is correct and the subsequent code is logically sound. The loop iterates through each unit in the team and checks if it is available before proceeding with further logic.142-148: The check for
TechnoExt::IsUnitAvailable(pLeaderUnit, true)is correct, and the logic to find a new team leader if the current one is not available is sound. The code correctly updates the team leader in theTeamExtdata.298-304: The logic to determine if the current focus is a valid target based on its air/ground status and whether the leader has the appropriate weapons is correct. The additional checks for immunity and mind control status are also appropriate.
309-315: The check for
TechnoExt::IsUnitAvailable(pFoot, true)is correct, and the subsequent logic handles various cases for different unit types, such as aircraft and naval units, correctly.485-491: The loop in
GreatestThreatcorrectly filters out objects that are not valid targets based on various conditions, including availability, immunity, and mind control status. The use ofTechnoExt::IsUnitAvailableis consistent with the rest of the codebase.1167-1173: The logic in
Mission_Attack_List1Randomcorrectly filters the list of potential targets based on their availability and whether they are allied or mind-controlled. The use ofTechnoExt::IsUnitAvailableis correct, and the loop correctly builds a list of valid indexes for random selection.
| #include <Ext/House/Body.h> | ||
| #include <Ext/WarheadType/Body.h> | ||
| #include <Ext/WeaponType/Body.h> | ||
| #include <Ext/BuildingType/Body.h> | ||
| #include <Ext/Script/Body.h> | ||
| #include <Utilities/EnumFunctions.h> | ||
|
|
There was a problem hiding this comment.
The inclusion of <Ext/BuildingType/Body.h> is redundant as it is already included on line 8. This could lead to potential compilation issues or confusion.
- #include <Ext/BuildingType/Body.h>Commitable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| #include <Ext/House/Body.h> | |
| #include <Ext/WarheadType/Body.h> | |
| #include <Ext/WeaponType/Body.h> | |
| #include <Ext/BuildingType/Body.h> | |
| #include <Ext/Script/Body.h> | |
| #include <Utilities/EnumFunctions.h> | |
| #include <Ext/House/Body.h> | |
| #include <Ext/WarheadType/Body.h> | |
| #include <Ext/WeaponType/Body.h> | |
| #include <Ext/Script/Body.h> | |
| #include <Utilities/EnumFunctions.h> | |
src/Ext/Techno/Body.cpp
Outdated
| return false; | ||
|
|
||
| if (pThis->GetCurrentMission() == Mission::Move) | ||
| { | ||
| pExt->CurrentRandomTarget = nullptr; | ||
| pExt->OriginalTarget = nullptr; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| if (pExt->CurrentRandomTarget && IsUnitAvailable(pExt->CurrentRandomTarget, false) && pThis->SpawnManager) | ||
| return false; | ||
|
|
||
| if (!pThis->Target && !IsUnitAvailable(abstract_cast<TechnoClass*>(pExt->OriginalTarget), false)) | ||
| { | ||
| pExt->OriginalTarget = nullptr; | ||
| return false; | ||
| } | ||
|
|
||
| if (pExt->OriginalTarget && !IsUnitAvailable(abstract_cast<TechnoClass*>(pExt->OriginalTarget), false)) | ||
| { | ||
| pExt->CurrentRandomTarget = nullptr; | ||
| pExt->OriginalTarget = nullptr; | ||
| } | ||
|
|
||
| if (pThis->GetCurrentMission() != Mission::Attack) | ||
| { | ||
| pExt->OriginalTarget = nullptr; | ||
| return false; | ||
| } | ||
|
|
||
| if (!pThis->Target) | ||
| return false; | ||
|
|
||
| if (pThis->DistanceFrom(pExt->OriginalTarget) > pWeapon->Range) | ||
| { | ||
| if (pThis->WhatAmI() == AbstractType::Building) | ||
| { | ||
| pThis->SetTarget(nullptr); | ||
| pExt->CurrentRandomTarget = nullptr; | ||
| pExt->OriginalTarget = nullptr; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| pThis->SetTarget(pExt->OriginalTarget); | ||
| } | ||
|
|
||
| if (pThis->DistanceFrom(pThis->Target) > pWeapon->Range) | ||
| { | ||
| pThis->SetTarget(pExt->OriginalTarget); | ||
| return false; | ||
| } | ||
|
|
||
| auto pRandomTarget = FindRandomTarget(pThis); | ||
|
|
||
| if (!pRandomTarget) | ||
| return false; | ||
|
|
||
| pExt->OriginalTarget = !pExt->OriginalTarget ? pThis->Target : pExt->OriginalTarget; | ||
| pExt->CurrentRandomTarget = pRandomTarget; | ||
| pThis->Target = pRandomTarget; | ||
|
|
||
| if (pThis->SpawnManager) | ||
| { | ||
| bool isFirstSpawn = true; | ||
|
|
||
| for (auto pSpawn : pThis->SpawnManager->SpawnedNodes) | ||
| { | ||
| if (!pSpawn->Unit) | ||
| continue; | ||
|
|
||
| TechnoClass* pSpawnTarget = nullptr; | ||
|
|
||
| auto pSpawnExt = TechnoExt::ExtMap.Find(pSpawn->Unit); | ||
| if (!pSpawnExt) | ||
| continue; | ||
|
|
||
| if (isFirstSpawn) | ||
| { | ||
| pSpawnTarget = pExt->CurrentRandomTarget; | ||
|
|
||
| if (pWeaponExt->RandomTarget_Spawners_MultipleTargets) | ||
| isFirstSpawn = false; | ||
| } | ||
| else | ||
| { | ||
| pSpawnTarget = FindRandomTarget(pThis); | ||
|
|
||
| if (!pSpawnTarget) | ||
| pSpawnTarget = abstract_cast<TechnoClass*>(pExt->OriginalTarget); | ||
| } | ||
|
|
||
| pSpawnExt->CurrentRandomTarget = pSpawnTarget; | ||
| pSpawnExt->OriginalTarget = pExt->OriginalTarget; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
The UpdateRandomTarget function has several issues:
- The function assumes that
pThis->GetWeapon(weaponIndex)will always return a valid pointer before dereferencing it to accessWeaponType. This could lead to a null pointer dereference ifSelectWeaponreturns an invalid index. Add a null check after getting the weapon. - The function uses
pThis->DistanceFrom(pExt->OriginalTarget)without checking ifpExt->OriginalTargetis a valid pointer, which could lead to a crash. Add a null check before usingpExt->OriginalTarget. - The function does not check if
pThis->SpawnManageris a valid pointer before using it in a loop, which could lead to a crash. Add a null check before the loop that iterates overpThis->SpawnManager->SpawnedNodes.
src/Ext/Techno/Body.cpp
Outdated
| TechnoClass* TechnoExt::FindRandomTarget(TechnoClass* pThis) | ||
| { | ||
| TechnoClass* selection = nullptr; | ||
|
|
||
| if (!pThis && !pThis->Target) | ||
| return selection; | ||
|
|
||
| int weaponIndex = pThis->SelectWeapon(pThis->Target); | ||
| auto pWeapon = pThis->GetWeapon(weaponIndex)->WeaponType; | ||
| if (!pWeapon) | ||
| return selection; | ||
|
|
||
| const auto pWeaponExt = WeaponTypeExt::ExtMap.Find(pWeapon); | ||
| if (!pWeaponExt || pWeaponExt->RandomTarget <= 0.0) | ||
| return selection; | ||
|
|
||
| const auto pExt = TechnoExt::ExtMap.Find(pThis); | ||
| if (!pExt) | ||
| return selection; | ||
|
|
||
| int retargetProbability = std::min((int)round(pWeaponExt->RandomTarget * 100), 100); | ||
| int dice = ScenarioClass::Instance->Random.RandomRanged(1, 100); | ||
|
|
||
| if (retargetProbability < dice) | ||
| return selection; | ||
|
|
||
| auto pThisType = pThis->GetTechnoType(); | ||
| int minimumRange = pWeapon->MinimumRange; | ||
| int range = pWeapon->Range; | ||
| int airRange = pWeapon->Range + pThisType->AirRangeBonus; | ||
| bool omniFire = pWeapon->OmniFire; | ||
| std::vector<TechnoClass*> candidates; | ||
| auto originalTarget = abstract_cast<TechnoClass*>(!pExt->OriginalTarget ? pThis->Target : pExt->OriginalTarget); | ||
| bool friendlyFire = pThis->Owner->IsAlliedWith(originalTarget); | ||
|
|
||
| // Looking for all valid targeting candidates | ||
| for (auto pCandidate : *TechnoClass::Array) | ||
| { | ||
| if (pCandidate == pThis | ||
| || !IsUnitAvailable(pCandidate, true) | ||
| || pThisType->Immune | ||
| || !EnumFunctions::IsTechnoEligible(pCandidate, pWeaponExt->CanTarget, true) | ||
| || (!pWeapon->Projectile->AA && pCandidate->IsInAir()) | ||
| || (!pWeapon->Projectile->AG && !pCandidate->IsInAir()) | ||
| || (!friendlyFire && (pThis->Owner->IsAlliedWith(pCandidate) || ScriptExt::IsUnitMindControlledFriendly(pThis->Owner, pCandidate))) | ||
| || pCandidate->TemporalTargetingMe | ||
| || pCandidate->BeingWarpedOut | ||
| || (pCandidate->GetTechnoType()->Underwater && pCandidate->GetTechnoType()->NavalTargeting == NavalTargetingType::Underwater_Never) | ||
| || (pCandidate->GetTechnoType()->Naval && pCandidate->GetTechnoType()->NavalTargeting == NavalTargetingType::Naval_None) | ||
| || (pCandidate->CloakState == CloakState::Cloaked && !pThisType->Naval) | ||
| || (pCandidate->InWhichLayer() == Layer::Underground)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| int distanceFromAttacker = pThis->DistanceFrom(pCandidate); | ||
| if (distanceFromAttacker < minimumRange) | ||
| continue; | ||
|
|
||
| if (omniFire) | ||
| { | ||
| if (pCandidate->IsInAir()) | ||
| range = airRange; | ||
|
|
||
| if (distanceFromAttacker <= range) | ||
| candidates.push_back(pCandidate); | ||
| } | ||
| else | ||
| { | ||
| int distanceFromOriginalTargetToCandidate = pCandidate->DistanceFrom(pThis->Target); | ||
| int distanceFromOriginalTarget = pThis->DistanceFrom(pThis->Target); | ||
|
|
||
| if (pCandidate->IsInAir()) | ||
| range = airRange; | ||
|
|
||
| if (distanceFromAttacker <= range && distanceFromOriginalTargetToCandidate <= distanceFromOriginalTarget) | ||
| candidates.push_back(pCandidate); | ||
| } | ||
| } | ||
|
|
||
| if (candidates.size() == 0) | ||
| return selection; | ||
|
|
||
| // Pick one new target from the list of targets inside the weapon range | ||
| dice = ScenarioClass::Instance->Random.RandomRanged(0, candidates.size() - 1); | ||
| selection = candidates.at(dice); | ||
|
|
||
| return selection; | ||
| } |
There was a problem hiding this comment.
The FindRandomTarget function has a potential issue:
- The function does not check if
pThis->Targetis a valid pointer before using it in calculations fordistanceFromOriginalTargetToCandidateanddistanceFromOriginalTarget. This could lead to a crash ifpThis->Targetis null. Add a null check before usingpThis->Target.
src/Ext/Techno/Body.cpp
Outdated
| .Process(this->DeployFireTimer) | ||
| .Process(this->ForceFullRearmDelay) | ||
| .Process(this->WHAnimRemainingCreationInterval) | ||
| .Process(this->OriginalTarget) | ||
| .Process(this->CurrentRandomTarget) | ||
| ; |
There was a problem hiding this comment.
The serialization of OriginalTarget and CurrentRandomTarget is added, but there is no version check to ensure backward compatibility with save files that do not have these fields. This could lead to issues when loading older save files. Implement a version check to handle the absence of these fields in older save files.
src/Ext/Script/Mission.Attack.cpp
Outdated
|
|
||
| pFocus = abstract_cast<TechnoClass*>(pTeam->Focus); | ||
|
|
||
| if (!IsUnitAvailable(pFocus, true)) | ||
| if (!TechnoExt::IsUnitAvailable(pFocus, true)) | ||
| { | ||
| pTeam->Focus = nullptr; | ||
| pFocus = nullptr; |
There was a problem hiding this comment.
The check for TechnoExt::IsUnitAvailable(pFocus, true) is correct, but the subsequent nulling of pTeam->Focus and pFocus should be inside the if block. The current placement outside the block will null the focus regardless of whether pFocus is available or not, which is likely unintended.
if (!TechnoExt::IsUnitAvailable(pFocus, true))
{
- pTeam->Focus = nullptr;
- pFocus = nullptr;
+ pTeam->Focus = nullptr;
+ pFocus = nullptr;
}Commitable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pFocus = abstract_cast<TechnoClass*>(pTeam->Focus); | |
| if (!IsUnitAvailable(pFocus, true)) | |
| if (!TechnoExt::IsUnitAvailable(pFocus, true)) | |
| { | |
| pTeam->Focus = nullptr; | |
| pFocus = nullptr; | |
| pFocus = abstract_cast<TechnoClass*>(pTeam->Focus); | |
| if (!TechnoExt::IsUnitAvailable(pFocus, true)) | |
| { | |
| pTeam->Focus = nullptr; | |
| pFocus = nullptr; | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (11)
- CREDITS.md (1 hunks)
- docs/New-or-Enhanced-Logics.md (1 hunks)
- docs/Whats-New.md (1 hunks)
- src/Ext/Script/Body.cpp (5 hunks)
- src/Ext/Techno/Body.Update.cpp (3 hunks)
- src/Ext/Techno/Body.cpp (3 hunks)
- src/Ext/Techno/Body.h (6 hunks)
- src/Ext/Techno/Hooks.Firing.cpp (2 hunks)
- src/Ext/Techno/Hooks.cpp (1 hunks)
- src/Ext/WeaponType/Body.cpp (2 hunks)
- src/Ext/WeaponType/Body.h (2 hunks)
Additional comments: 26
src/Ext/WeaponType/Body.h (3)
- 42-42: The addition of
RandomTargetas a double and its initialization to0.0is appropriate for representing a percentage or range value.- 43-43: The addition of
RandomTarget_DistributeBurstas a boolean and its initialization tofalseis logical for a default behavior.- 44-44: The addition of
RandomTarget_Spawners_MultipleTargetsas a boolean and its initialization tofalsemakes sense to require explicit enabling for this behavior.src/Ext/Techno/Body.h (5)
- 9-9: The inclusion of
EnumFunctions.his appropriate for the functionality introduced in this file.- 42-44: The addition of
OriginalTarget,ResetRandomTarget, andCurrentRandomTargetmembers, along with their initializations, is logical for managing the state of random targeting.- 86-86: The addition of the
UpdateRandomTargetsfunction is appropriate for implementing the random target feature.- 131-131: The addition of the
IsUnitAvailablestatic function is appropriate for checking unit availability in the context of random targeting.- 158-160: The addition of
UpdateRandomTarget,FindRandomTarget, andIsValidTechnostatic functions is logical for managing random target selection and validation.src/Ext/WeaponType/Body.cpp (2)
- 62-64: The modifications to
LoadFromINIFileto support reading the new random target properties from INI files are appropriate and necessary for external configuration.- 92-94: The modifications to
Serializeto support serializing and deserializing the new random target properties are crucial for saving and loading these properties.src/Ext/Techno/Hooks.cpp (1)
- 9-9: The inclusion of
Script/Body.his appropriate for the functionality introduced in this file.CREDITS.md (1)
- 133-133: The addition of the
Burst.Retargetfeature by Starkku is correctly formatted and consistent with the rest of the document.src/Ext/Techno/Body.cpp (3)
- 8-9: Adding new includes for
WeaponTypeandScriptextensions is necessary for the new functionalities introduced in this PR. Ensure that these headers are properly guarded against multiple inclusions and that they do not introduce circular dependencies.- 503-595: The
FindRandomTargetfunction is responsible for finding a random target within the attacker's area. It's a critical part of the retargeting feature and seems to be well-implemented with checks for various conditions. However, ensure that the function correctly handles edge cases, such as when no valid targets are found or when specific targeting restrictions apply.
- Verify that the function's logic correctly accounts for all game rules and scenarios.
- Consider adding comments to complex conditional checks to improve readability.
- 597-611: The
IsValidTechnofunction checks if a techno is valid for targeting or other actions. It's a good practice to centralize such checks in a single function to avoid code duplication and ensure consistency. However, consider adding more detailed comments explaining each condition for future maintainability.src/Ext/Techno/Body.Update.cpp (2)
- 10-10: The inclusion of
<Ext/Script/Body.h>seems necessary for the new functionality introduced. Ensure that this header is indeed used within the file to avoid unnecessary compilation dependencies.- 751-809: The
UpdateRandomTargetsfunction introduces logic for handling random target updates for units. Here are a few observations and suggestions:
Correctness and Logic: The function checks for the validity of the current random target and updates or resets it based on various conditions. Ensure that all conditions accurately reflect the intended logic, especially the checks for
IsUnitAvailableand the handling ofSpawnManager's spawned nodes.Performance: The function iterates over
SpawnManager's spawned nodes within a loop (lines 780-799). Consider the potential performance impact if there are a large number of spawned nodes. If possible, look for optimizations or ensure that this operation does not become a bottleneck.Maintainability: The function is relatively large and handles multiple responsibilities. Consider breaking it down into smaller, more focused functions if the logic becomes too complex or if there are distinct sections that could be modularized. This will improve readability and maintainability.
Error Handling: Ensure that there is adequate error handling, especially for external calls that might fail or return unexpected results.
Overall, the function seems to align with the objectives of the PR, enhancing gameplay dynamics by allowing weapons to select targets randomly within defined parameters. Just ensure that the implementation details are thoroughly tested, especially the interactions with different unit types and the
SpawnManager.src/Ext/Techno/Hooks.Firing.cpp (1)
- 868-1110: The implementation of the
Burst.Retargetfeature and handling of random target selection for units introduces several hooks and logic to update targets under specific conditions. While the overall approach aligns with the PR objectives, there are a few areas that could benefit from further refinement:
Consistency in Target Validation: The checks for valid targets are repeated across multiple hooks (
SpawnerManagerClassAI_SwitchCase3,SpawnerManagerClassAI_SwitchCase4,SpawnerManagerClassAI_SpawnControlStatus3,TechnoClass_FireAt_BulletNewTarget). Consider abstracting these checks into a utility function to reduce code duplication and improve maintainability.Error Handling: Ensure that all external calls, especially those interacting with game objects and their properties, are safely handled. For instance, checks for null pointers and the existence of required extensions should be thorough to prevent crashes.
Performance Considerations: The loops iterating over all current objects in
AIMissionClassUAEXXZ_StopSelected_ClearRetargetsand similar hooks could potentially impact performance, especially in large-scale battles. Evaluate if there are more efficient ways to achieve the desired functionality, such as maintaining a list of units with random targets that need to be cleared.Documentation and Comments: While the code changes are significant, there's a noticeable lack of comments explaining the logic and decisions behind certain implementations. Adding detailed comments would greatly aid in understanding the code, especially for complex logic related to targeting and weapon selection.
Testing and Validation: Given the complexity of the changes and their potential impact on gameplay, thorough testing is recommended. This includes testing with various unit types, weapon configurations, and game scenarios to ensure that the
Burst.Retargetfeature behaves as expected without introducing bugs or balance issues.Consider the above points to refine the implementation, focusing on code maintainability, performance, and robustness. If you need assistance with specific improvements or testing strategies, feel free to ask.
src/Ext/Script/Body.cpp (6)
- 420-420: The call to
TechnoExt::IsUnitAvailable(pLeaderUnit, true)correctly reflects the function's new location. However, ensure that theTechnoExtclass and theIsUnitAvailablefunction are properly implemented to handle the logic previously managed byScriptExt.- 455-455: The repeated calls to
TechnoExt::IsUnitAvailable(pUnit, true)within the loop correctly use the updated function location. It's important to verify that theTechnoExtimplementation ofIsUnitAvailablemaintains the same logic and error handling as its predecessor inScriptExt.- 753-753: The usage of
TechnoExt::IsUnitAvailablein theMoveMissionEndStatusfunction correctly reflects the function's relocation. As with other instances, ensure that theTechnoExtversion ofIsUnitAvailableis fully compatible with the expected behavior in this context.- 1085-1085: The call to
TechnoExt::IsUnitAvailableinFindTheTeamLeaderis correctly updated to match the function's new location. This is a critical part of determining the team leader, so it's essential to ensure that theTechnoExtimplementation ofIsUnitAvailableperforms as expected.- 417-423: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file includes necessary headers and maintains a clean structure, which is good for maintainability. Ensure that all included headers are used within this file to avoid unnecessary dependencies.
- 417-423: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Given the extensive use of
TechnoExt::IsUnitAvailablefollowing its move fromScriptExt, consider adding a brief comment at each call site briefly explaining the criteria for unit availability. This would enhance readability and maintainability by providing context directly within theScriptExtfile, especially for developers unfamiliar with theTechnoExtimplementation.docs/Whats-New.md (1)
- 351-351: The addition of the
Burst.Retargetfeature as highlighted in the summary aligns well with the detailed changelog provided. This feature, along with the other listed changes, showcases significant enhancements to the project.docs/New-or-Enhanced-Logics.md (1)
- 1277-1292: The documentation provides a clear explanation of the
RandomTargetfeature for projectiles, including its behavior with missiles, cannons, lasers, and spawners. It also outlines howOmniFireaffects target selection and the special handling for spawners withRandomTarget.Spawners.MultipleTargets=true. This feature enhances gameplay by allowing for more dynamic interactions between units and their targets. The provided code snippet correctly demonstrates how to configure this feature inrulesmd.ini.
src/Ext/Techno/Body.cpp
Outdated
|
|
||
| return false; | ||
| } | ||
|
|
||
| if (!IsValidTechno(pExt->CurrentRandomTarget)) | ||
| pExt->CurrentRandomTarget = nullptr; | ||
|
|
||
| if (pExt->OriginalTarget && !IsValidTechno(static_cast<TechnoClass*>(pExt->OriginalTarget))) | ||
| pExt->OriginalTarget = nullptr; | ||
|
|
||
| if (pThis->Target && !IsValidTechno(static_cast<TechnoClass*>(pThis->Target))) | ||
| pThis->SetTarget(nullptr); | ||
|
|
||
| if (pThis->Target && pExt->CurrentRandomTarget && pThis->CurrentMission == Mission::Guard) | ||
| pThis->ForceMission(Mission::Attack); | ||
|
|
||
| if (pExt->CurrentRandomTarget && pThis->SpawnManager) | ||
| return false; | ||
|
|
||
| if (!pThis->Target && !pExt->OriginalTarget) | ||
| return false; | ||
|
|
||
| if (!pExt->OriginalTarget) | ||
| { | ||
| pExt->CurrentRandomTarget = nullptr; | ||
| pExt->OriginalTarget = nullptr; | ||
| } | ||
|
|
||
| if (pExt->CurrentRandomTarget && pThis->GetCurrentMission() != Mission::Attack) | ||
| { | ||
| pExt->CurrentRandomTarget = nullptr; | ||
| pThis->SetTarget(pExt->OriginalTarget ? pExt->OriginalTarget : nullptr); | ||
| pExt->OriginalTarget = nullptr; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| if (!pThis->Target) | ||
| return false; | ||
|
|
||
| if (pThis->DistanceFrom(pExt->OriginalTarget) > pWeapon->Range || pThis->DistanceFrom(pThis->Target) > pWeapon->Range) | ||
| { | ||
| if (pThis->WhatAmI() == AbstractType::Building) | ||
| { | ||
| pThis->SetTarget(nullptr); | ||
| pExt->CurrentRandomTarget = nullptr; | ||
| pExt->OriginalTarget = nullptr; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| pThis->SetTarget(pExt->OriginalTarget); | ||
| } | ||
|
|
||
| auto pRandomTarget = FindRandomTarget(pThis); | ||
|
|
||
| if (!pRandomTarget) | ||
| return false; | ||
|
|
||
| pExt->OriginalTarget = !pExt->OriginalTarget ? pThis->Target : pExt->OriginalTarget; | ||
| pExt->CurrentRandomTarget = pRandomTarget; | ||
| pThis->Target = pRandomTarget; | ||
|
|
||
| if (pThis->SpawnManager) | ||
| { | ||
| bool isFirstSpawn = true; | ||
|
|
||
| for (auto pSpawn : pThis->SpawnManager->SpawnedNodes) | ||
| { | ||
| if (!pSpawn->Unit) | ||
| continue; | ||
|
|
||
| TechnoClass* pSpawnTarget = nullptr; | ||
|
|
||
| auto pSpawnExt = TechnoExt::ExtMap.Find(pSpawn->Unit); | ||
| if (!pSpawnExt) | ||
| continue; | ||
|
|
||
| if (isFirstSpawn) | ||
| { | ||
| pSpawnTarget = pExt->CurrentRandomTarget; | ||
|
|
||
| if (pWeaponExt->RandomTarget_Spawners_MultipleTargets) | ||
| isFirstSpawn = false; | ||
| } | ||
| else | ||
| { | ||
| pSpawnTarget = FindRandomTarget(pThis); | ||
|
|
||
| if (!pSpawnTarget) | ||
| pSpawnTarget = static_cast<TechnoClass*>(pExt->OriginalTarget); | ||
| } | ||
|
|
||
| pSpawnExt->CurrentRandomTarget = pSpawnTarget; | ||
| pSpawnExt->OriginalTarget = pExt->OriginalTarget; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
The UpdateRandomTarget function is complex and handles multiple scenarios. It's well-structured but could benefit from breaking down into smaller functions for better readability and maintainability. Additionally, ensure that all dereferenced pointers are checked for null before use to prevent potential null pointer dereferences.
- Consider refactoring to split the function into smaller, more focused functions.
- Add null checks for pointers like
pWeaponandpThis->GetWeapon(weaponIndex)before dereferencing.
| .Process(this->OriginalTarget) | ||
| .Process(this->ResetRandomTarget) | ||
| .Process(this->CurrentRandomTarget) |
There was a problem hiding this comment.
Serialization of new data members OriginalTarget, ResetRandomTarget, and CurrentRandomTarget is crucial for saving and loading game states. Ensure that version checks are implemented to maintain backward compatibility with older save files, as mentioned in previous comments.
void TechnoExt::ExtData::Serialize(T& Stm)
{
Stm
.Process(this->TypeExtData)
...
+ // Implement version checks here to ensure backward compatibility
.Process(this->OriginalTarget)
.Process(this->ResetRandomTarget)
.Process(this->CurrentRandomTarget)
;
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| .Process(this->OriginalTarget) | |
| .Process(this->ResetRandomTarget) | |
| .Process(this->CurrentRandomTarget) | |
| // Implement version checks here to ensure backward compatibility | |
| .Process(this->OriginalTarget) | |
| .Process(this->ResetRandomTarget) | |
| .Process(this->CurrentRandomTarget) |
| @@ -5,6 +5,8 @@ | |||
| #include <Ext/House/Body.h> | |||
| #include <Ext/WarheadType/Body.h> | |||
| #include <Ext/WeaponType/Body.h> | |||
| #include <Ext/BuildingType/Body.h> | |||
There was a problem hiding this comment.
The inclusion of <Ext/BuildingType/Body.h> is redundant as it is already included on line 5. This could lead to potential compilation issues or confusion.
- #include <Ext/BuildingType/Body.h>Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| #include <Ext/BuildingType/Body.h> |
docs/Whats-New.md
Outdated
| - Customizable straight trajectory detonation & snap distance and pass-through option (by Starkku) | ||
| - Airstrike & spy plane fixed spawn distance & height (by Starkku) | ||
| - Allow enabling application of `Verses` and `PercentAtMax` for negative damage (by Starkku) | ||
| - `Burst.Retarget` for assigning a new target in each projectile (by FS-21) | ||
| - In addition to `PlacementGrid.Translucency`, allow to set the transparency of the grid when PlacementPreview is enabled, using the `PlacementGrid.TranslucencyWithPreview` tag (by Belonit). | ||
| - Show briefing screen on singleplayer mission start (by Starkku) | ||
| - Allow setting mission par times and related messages in `missionmd.ini` (by Starkku) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [3-3]
There's a spelling mistake in the word "accomodate". It should be spelled as "accommodate".
- accomodate
+ accommodate
I don't know if this was supposed to be fixed or not, but i got to test it in the newest build available, and the issue persists. Cloaked units still cause spawners to fly away. |
|
Sorry, I think I forgot about that case x'D When I arrive home I would like to update the code. |
|
No need to hurry. On a side note:
There's something else going with this, it seems. I don't know how did i manage to make it not happen without Phobos before, but i retested it now and it does happen, and i didn't really change anything for the unit in question (at least i don't think i did). It appears to happen randomly, and the more infantry units are in a group, the more noticeable it becomes. Either way, no need to fix anything with this one, I'll probably report that as a separate issue. |
And renaming some events
The major part of the code was rewritten.
OmniFire=yeswill make selectable any targets around the firer, limited by the weapon range.OmniFire=nowill force the firer to pick targets in an area composed by the firer's weapon range around the original target intersected with the firer's weapon range around the firer.RandomTarget.Spawners.MultipleTargets=truegives each spawner it's own target.[SOMEWEAPON] ; WeaponType
RandomTarget=0.0 ; double or percentage
RandomTarget.Spawners.MultipleTargets=false ; boolean
Summary by CodeRabbit
New Features
Burst.Retargetallowing projectiles to retarget after each burst.VersesandPercentAtMaxfor negative damage.Enhancements
Documentation
New-or-Enhanced-Logics.mdwith examples and conditions for new projectile features.Whats-New.md.Refactor
TechnoExtclass.IsUnitAvailablefunction fromScriptExtclass.Bug Fixes