Change target Owner on warhead impact #1118
Conversation
added ownerchanger to warheads
Immunities update
|
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. |
Fryone
force-pushed
the
fryone/owner_changer
branch
from
0cf4daa to
3de494f
Compare
WalkthroughThe latest update introduces the ability for warheads to change the ownership of targets upon detonation, incorporating mind control effects and various conditions such as immunity, armor type, health thresholds, and elite status. This enhancement allows for more strategic depth in gameplay, enabling players to turn enemy units to their side under specific circumstances. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- CREDITS.md (1 hunks)
- docs/New-or-Enhanced-Logics.md (1 hunks)
- docs/Whats-New.md (1 hunks)
- src/Ext/TechnoType/Body.h (2 hunks)
- src/Ext/WarheadType/Body.cpp (3 hunks)
- src/Ext/WarheadType/Body.h (3 hunks)
- src/Ext/WarheadType/Detonate.cpp (2 hunks)
Additional comments: 14
src/Ext/WarheadType/Body.h (2)
44-48: The new properties
ChangeOwner,ChangeOwner_MindControl,ChangeOwner_Anim,ChangeOwner_Threshold, andChangeOwner_AffectEliteshave been added to theWarheadTypeExt::ExtDataclass. These properties are initialized with default values in the constructor. The naming convention is consistent, and the properties are well-documented in the PR objectives.263-263: The method
ApplyOwnerChangehas been declared but not defined in this file. Ensure that the implementation of this method correctly handles the logic for changing the ownership of units as described in the PR objectives.Verification successful
The search for the
ApplyOwnerChangemethod definition has yielded results. The method is indeed defined insrc/Ext/WarheadType/Detonate.cppas indicated by the output:src/Ext/WarheadType/Detonate.cpp:void WarheadTypeExt::ExtData::ApplyOwnerChange(HouseClass* pHouse, TechnoClass* pTarget)This confirms that the method
ApplyOwnerChangehas been both declared inBody.hand defined inDetonate.cpp, which aligns with the PR objectives that describe the implementation of new logic for changing unit ownership upon warhead impact.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the definition of ApplyOwnerChange in the codebase to ensure it's implemented correctly. rg --type cpp "void ExtData::ApplyOwnerChange"Length of output: 48
Script:
#!/bin/bash # Broaden the search for the ApplyOwnerChange method definition in the codebase. rg "ApplyOwnerChange"Length of output: 315
src/Ext/TechnoType/Body.h (1)
- 185-185: The new property
ChangeOwnerImmunitieshas been added to theTechnoTypeExt::ExtDataclass. ThisNullableVector<int>is intended to store owner immunities. Ensure that the usage of this property is consistent with the logic described in the PR objectives and that it is properly handled in the implementation.CREDITS.md (1)
- 305-305: The addition of "Change target Owner on warhead impact" under the contributions of Fryone is consistent with the changes made in this PR. It's good to see contributions being acknowledged in the project documentation.
src/Ext/WarheadType/Detonate.cpp (2)
138-140: The conditional block added to
DetonateOnOneUnitchecks theChangeOwnerflag and callsApplyOwnerChangeif set. This is consistent with the PR objectives to change unit ownership upon warhead impact. Ensure that theApplyOwnerChangefunction is implemented correctly and handles all edge cases as intended.256-309: The
ApplyOwnerChangefunction has been added to handle the logic for applying the owner change to the target unit. The function checks various conditions such as health threshold, mind control immunity, and elite status before applying the change. Ensure that the logic correctly implements the feature as described in the PR objectives and that it properly interacts with other game mechanics.src/Ext/WarheadType/Body.cpp (4)
130-134: The new properties for the ownership change feature have been added to the
WarheadTypeExt::ExtDataclass. Ensure that the corresponding INI keys are documented and that their behavior is consistent with the rest of the system.130-134: Verify that the default values for the new INI keys are set appropriately and that they behave as expected when not defined in the INI file.
315-319: Ensure that the serialization and deserialization of the new properties are correctly implemented and that they are compatible with savegame versions.
Verification successful
The shell script output confirms that the new properties related to the warhead's ownership change feature are both read from the INI file (deserialization) and processed for saving/loading (serialization) in
src/Ext/WarheadType/Body.cpp. This indicates that serialization and deserialization are correctly implemented for these properties.* 286-286: Confirm that the inclusion of the `ChangeOwner` flag in `PossibleCellSpreadDetonate` aligns with the intended logic for cell spread detonation and does not introduce any unintended side effects.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the serialization and deserialization of the new properties are correctly implemented. # This script checks for the presence of serialization code for the new properties. rg --type cpp "this->ChangeOwner" src/Ext/WarheadType/Body.cpp rg --type cpp "this->ChangeOwner_MindControl" src/Ext/WarheadType/Body.cpp rg --type cpp "this->ChangeOwner_Anim" src/Ext/WarheadType/Body.cpp rg --type cpp "this->ChangeOwner_Threshold" src/Ext/WarheadType/Body.cpp rg --type cpp "this->ChangeOwner_AffectElites" src/Ext/WarheadType/Body.cppLength of output: 1492
docs/Whats-New.md (3)
352-352: The documentation correctly reflects the addition of the feature allowing warheads to change the ownership of units upon impact. This is a significant feature and should be highlighted appropriately in the documentation.
352-352: Ensure that the new feature is documented consistently across all relevant sections of the documentation to maintain clarity for modders.
352-352: The changelog entry for the new feature of changing target ownership on warhead impact is placed under the correct version section and is described clearly.
docs/New-or-Enhanced-Logics.md (1)
- 1202-1224: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1-121]
The documentation updates in
New-or-Enhanced-Logics.mdare consistent with the PR objectives and AI-generated summaries. The new warhead properties such asIsOwnerChanger.SetAsMindControl,IsOwnerChanger.AffectElites, andIsOwnerChanger.HealthThresholdare well-documented and provide clear instructions for modders on how to use these features inrulesmd.ini.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- CREDITS.md (1 hunks)
- docs/New-or-Enhanced-Logics.md (1 hunks)
- docs/Whats-New.md (1 hunks)
- src/Ext/TechnoType/Body.h (2 hunks)
- src/Ext/WarheadType/Body.cpp (3 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional comments: 5
src/Ext/TechnoType/Body.h (1)
- 365-365: Initialization of
ChangeOwnerImmunitiesin the constructor is correct and follows the class's initialization pattern.CREDITS.md (1)
- 312-312: The addition to the credits for the change target owner on warhead impact feature is accurate and properly acknowledges the contribution.
src/Ext/WarheadType/Body.cpp (2)
- 286-286: The addition of
ChangeOwnertoPossibleCellSpreadDetonateimplies that this feature will be considered in the detonation logic that respects cell spread. Verify that this integration is consistent with the intended behavior and that all necessary conditions for owner change are properly evaluated in the detonation logic.- 315-319: Serialization of the new properties is correctly implemented. However, ensure that the serialization and deserialization processes are thoroughly tested, especially in scenarios involving save and load cycles, to prevent data corruption or loss.
docs/New-or-Enhanced-Logics.md (1)
- 1205-1220: The documentation correctly introduces the new warhead properties for changing unit ownership upon impact. However, ensure that the implementation details, such as how
IsOwnerChanger.HealthThresholdis applied and the behavior whenIsOwnerChanger.SetAsMindControlis enabled, are clearly explained in the actual implementation or additional documentation to avoid confusion.
src/Ext/WarheadType/Body.cpp
Outdated
| this->ChangeOwner.Read(exINI, pSection, "IsOwnerChanger"); | ||
| this->ChangeOwner_MindControl.Read(exINI, pSection, "IsOwnerChanger.SetAsMindControl"); | ||
| this->ChangeOwner_Anim.Read(exINI, pSection, "IsOwnerChanger.ControlAnim"); | ||
| this->ChangeOwner_Threshold.Read(exINI, pSection, "IsOwnerChanger.HealthThreshold"); | ||
| this->ChangeOwner_AffectElites.Read(exINI, pSection, "IsOwnerChanger.AffectElites"); |
There was a problem hiding this comment.
Ensure that the default values for ChangeOwner, ChangeOwner_MindControl, ChangeOwner_Anim, ChangeOwner_Threshold, and ChangeOwner_AffectElites are set appropriately in the constructor or elsewhere if not done so. This is crucial for maintaining predictable behavior when these properties are not explicitly defined in the INI files.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (5)
- CREDITS.md (1 hunks)
- docs/Whats-New.md (1 hunks)
- src/Ext/WarheadType/Body.cpp (3 hunks)
- src/Ext/WarheadType/Body.h (3 hunks)
- src/Ext/WarheadType/Detonate.cpp (2 hunks)
Additional comments: 6
src/Ext/WarheadType/Body.h (1)
- 44-48: The new properties related to changing ownership behavior (
ChangeOwner,ChangeOwner_MindControl,ChangeOwner_Anim,ChangeOwner_Threshold,ChangeOwner_AffectElites) are well-defined and follow the naming conventions of the existing codebase. However, ensure that the default values assigned to these properties align with the intended functionality and that they are thoroughly documented to guide users on their usage.CREDITS.md (1)
- 313-313: The acknowledgment of the contribution for the feature to change the target owner on warhead impact is a good practice, ensuring that contributors are recognized for their work. It's important to keep this section up-to-date as the project evolves and more contributors add value to the project.
src/Ext/WarheadType/Detonate.cpp (1)
- 138-139: The integration of
ApplyOwnerChangewithinDetonateOnOneUnitmethod is correctly placed and logically sound, ensuring that ownership change logic is applied under theChangeOwnercondition. This aligns with the PR's objective to enable unit ownership change upon warhead impact.src/Ext/WarheadType/Body.cpp (3)
- 130-134: The implementation of new properties related to changing the ownership of units upon warhead impact (
ChangeOwner,ChangeOwner_MindControl,ChangeOwner_Anim,ChangeOwner_Threshold, andChangeOwner_AffectElites) is correctly done. However, ensure that default values for these properties are set appropriately in the constructor or elsewhere if not explicitly defined in the INI files. This is crucial for maintaining predictable behavior.- 250-250: The addition of
ChangeOwnertoPossibleCellSpreadDetonateis a logical step to ensure that the new ownership change functionality is considered when determining if a warhead should detonate across the entire map. This change appears to be correctly implemented.- 279-283: The serialization logic for the new properties (
ChangeOwner,ChangeOwner_MindControl,ChangeOwner_Anim,ChangeOwner_Threshold, andChangeOwner_AffectElites) is correctly implemented. This ensures that these properties are correctly saved and loaded, maintaining the state of these configurations across sessions.
| void ApplyConvert(HouseClass* pHouse, TechnoClass* pTarget); | ||
| void ApplyLocomotorInfliction(TechnoClass* pTarget); | ||
| void ApplyLocomotorInflictionReset(TechnoClass* pTarget); | ||
| void ApplyOwnerChange(HouseClass* pHouse, TechnoClass* pTarget); |
There was a problem hiding this comment.
The addition of the ApplyOwnerChange method is crucial for implementing the new feature. It's important to ensure that this method is called at the appropriate places within the warhead detonation logic to apply the ownership change effects correctly. Additionally, consider adding error handling within this method to gracefully handle any potential issues that may arise during its execution, such as null references or invalid parameters.
Consider adding error handling within the ApplyOwnerChange method to ensure robustness.
|
The
|
weird, can you please provide settings for owner changer? As I tested, my anim is following techno as should |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/Ext/WarheadType/Detonate.cpp (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/Ext/WarheadType/Detonate.cpp
|
It's a bit messed up. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/Ext/WarheadType/Detonate.cpp (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/Ext/WarheadType/Detonate.cpp
|
I'm not touching MC, instead I'm using PermaMC as it's much simpler. The only issue is that ownerchanged units without PermaMC set, allowed to enter in transports, makes their attached animation (if its looping) dissapear on leaving transport. So I'll just drop it for ares' attached anim instead. |
'IsOwnerChanger.ControlAnim' -> changed to 'IsOwnerChanger.MindControlAnim' to use specifically as custom anim for permanent MC
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- docs/New-or-Enhanced-Logics.md (1 hunks)
- src/Ext/WarheadType/Body.cpp (3 hunks)
- src/Ext/WarheadType/Body.h (3 hunks)
- src/Ext/WarheadType/Detonate.cpp (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/Ext/WarheadType/Body.cpp
- src/Ext/WarheadType/Body.h
- src/Ext/WarheadType/Detonate.cpp
Additional comments: 1
docs/New-or-Enhanced-Logics.md (1)
- 1205-1221: The documentation for the new warhead properties related to changing unit ownership upon impact is clear and well-detailed. It correctly outlines the purpose and usage of each property, such as
IsOwnerChanger,IsOwnerChanger.SetAsMindControl,IsOwnerChanger.AffectElites, andIsOwnerChanger.HealthThreshold. These additions provide significant gameplay mechanics enhancements, allowing for more strategic depth in unit control and interactions.One minor suggestion for improvement is to include examples or scenarios where these properties could be particularly useful or impactful in gameplay. This could help mod developers better understand the potential applications and benefits of utilizing these new warhead properties in their mods.
Overall, the documentation is well-written and informative, providing a solid foundation for mod developers to integrate these new features into their projects.
The nightly build seems to be failing so I can't test your latest changes but here's what I have I also tested with EMP_FX01 and MINDANIMR (unchanged from vanilla). |
|
Yep, as I thought, now 'ControlAnim' changed to 'MindControlAnim' and will follow target as it should. |
DeathFishAtEase
force-pushed
the
develop
branch
from
2128644 to
799ea22
Compare
Starkku
force-pushed
the
develop
branch
2 times, most recently
from
b429215 to
280b1c8
Compare
TaranDahl
left a comment
TaranDahl
left a comment
There was a problem hiding this comment.
Fix the following things, then this can be merged.
TaranDahl
added
Fix and merge this
Needs testing
⚙️T1
# Conflicts: # CREDITS.md # YRpp # docs/New-or-Enhanced-Logics.md # docs/Whats-New.md # src/Ext/TechnoType/Body.h # src/Ext/WarheadType/Body.cpp # src/Ext/WarheadType/Body.h # src/Ext/WarheadType/Detonate.cpp
|
changed tag names to I'm not sure about whether the elite check should be removed tho. There're only a few warhead effects which really need it, so maybe it don't have to be a general check |
|
Is it worth to add timed control, similar to Dune's Deviator? |
|
I think it could be useful, but we should also think about how to deal with the interaction of multiple ChangeOwner warheads and other owner changer effects (mind control, capture etc). Maybe it's an easier way to just make the timer be interrupted by another change house event (there's existing hook for that too) |
5 participants
Change target Owner on impact
ChangeOwner.SetAsMindControlmakes the effect work like permanent mind control, which respectsImmuneToPsionics.ChangeOwner.MindControlAnimdetermines the mind control anim of this effect, which respectsMindControlRingOffset.ChangeOwner.AffectElitesdetermines if elite units can be affected by owner change.In
rulesmd.ini: