Buildings as destroyable pathfinding obstacles #1204
Conversation
WalkthroughThe overall change focuses on enhancing the pathfinding and blockage handling capabilities within a software project, likely a game or simulation. It introduces improved logic for units navigating around obstacles, considering weapon capabilities for clearing paths. A new source file adds specific functionality for assessing target feasibility based on weapon properties. Additionally, a configuration option is introduced to toggle these improvements, and code readability is slightly enhanced in one instance. 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 (10)
- CREDITS.md (1 hunks)
- Phobos.vcxproj (1 hunks)
- docs/New-or-Enhanced-Logics.md (1 hunks)
- docs/Whats-New.md (1 hunks)
- src/Ext/Techno/Hooks.Blockage.cpp (1 hunks)
- src/Ext/Techno/Hooks.cpp (1 hunks)
- src/Ext/WeaponType/Body.cpp (2 hunks)
- src/Ext/WeaponType/Body.h (2 hunks)
- src/Phobos.INI.cpp (2 hunks)
- src/Phobos.h (1 hunks)
Files not reviewed due to errors (1)
- docs/Whats-New.md (Error: unable to parse review)
Files skipped from review due to trivial changes (1)
- src/Ext/Techno/Hooks.cpp
Additional comments: 9
src/Phobos.h (1)
- 70-70: LGTM!
src/Ext/WeaponType/Body.h (1)
- 44-44: LGTM!
src/Ext/WeaponType/Body.cpp (2)
- 64-64: LGTM!
- 92-92: LGTM!
src/Ext/Techno/Hooks.Blockage.cpp (1)
- 1-263: LGTM!
src/Phobos.INI.cpp (2)
- 35-35: LGTM!
- 124-124: LGTM!
CREDITS.md (1)
- 222-222: LGTM!
Phobos.vcxproj (1)
- 54-54: LGTM!
|
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. |
|
Ok, it works without major problems with my mod pretty well. The only flaw I found is that units attacks terrain objects with IsPassable=yes, while not doing this with build 38.
|
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- src/Ext/Techno/Body.cpp (2 hunks)
- src/Ext/Techno/Body.h (1 hunks)
- src/Ext/Techno/Hooks.Blockage.cpp (1 hunks)
- src/Ext/Techno/Hooks.Firing.cpp (3 hunks)
- src/Ext/TerrainType/Hooks.Passable.cpp (1 hunks)
Files not reviewed due to errors (1)
- (no review received)
Files skipped from review as they are similar to previous changes (1)
- src/Ext/Techno/Hooks.Blockage.cpp
Additional comments: 3
src/Ext/Techno/Body.h (1)
- 146-146: The addition of
GetWeaponIndexAgainstWallmethod is correctly implemented.src/Ext/Techno/Body.cpp (2)
- 5-5: Including
OverlayTypeClass.his necessary for the new functionality.- 9-9: Including
Ext/WeaponType/Body.his necessary for accessing extended weapon type properties.
Starkku
force-pushed
the
feature/blockage-pathfinding
branch
from
0999d75
to
e7b5465
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (14)
- CREDITS.md (1 hunks)
- Phobos.vcxproj (1 hunks)
- docs/New-or-Enhanced-Logics.md (1 hunks)
- docs/Whats-New.md (1 hunks)
- src/Ext/Techno/Body.cpp (2 hunks)
- src/Ext/Techno/Body.h (1 hunks)
- src/Ext/Techno/Hooks.Blockage.cpp (1 hunks)
- src/Ext/Techno/Hooks.Firing.cpp (3 hunks)
- src/Ext/Techno/Hooks.cpp (1 hunks)
- src/Ext/TerrainType/Hooks.Passable.cpp (1 hunks)
- src/Ext/WeaponType/Body.cpp (2 hunks)
- src/Ext/WeaponType/Body.h (2 hunks)
- src/Phobos.INI.cpp (2 hunks)
- src/Phobos.h (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- CREDITS.md
- Phobos.vcxproj
- docs/New-or-Enhanced-Logics.md
- docs/Whats-New.md
- src/Ext/Techno/Body.cpp
- src/Ext/Techno/Body.h
- src/Ext/Techno/Hooks.Blockage.cpp
- src/Ext/Techno/Hooks.Firing.cpp
- src/Ext/Techno/Hooks.cpp
- src/Ext/TerrainType/Hooks.Passable.cpp
- src/Ext/WeaponType/Body.cpp
- src/Ext/WeaponType/Body.h
- src/Phobos.INI.cpp
- src/Phobos.h
src/Ext/Techno/Body.cpp
Outdated
| else if (weaponIndex == -1) | ||
| return 0; | ||
|
|
||
| if (!pWeapon || (!pWeapon->Warhead->Wall && (!pWeapon->Warhead->Wood || pWallOverlayType->Armor != Armor::Wood) |
There was a problem hiding this comment.
Would be nice to pull out these complex conditions into named bools before the if
Starkku
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files selected for processing (12)
- CREDITS.md (1 hunks)
- YRpp (1 hunks)
- docs/New-or-Enhanced-Logics.md (1 hunks)
- docs/Whats-New.md (1 hunks)
- src/Ext/Building/Body.cpp (1 hunks)
- src/Ext/Building/Body.h (1 hunks)
- src/Ext/Building/Hooks.cpp (1 hunks)
- src/Ext/BuildingType/Body.cpp (2 hunks)
- src/Ext/BuildingType/Body.h (2 hunks)
- src/Ext/Techno/Body.cpp (2 hunks)
- src/Ext/Techno/Body.h (1 hunks)
- src/Ext/Techno/Hooks.Firing.cpp (3 hunks)
Files skipped from review due to trivial changes (1)
- YRpp
Additional comments: 11
src/Ext/Building/Hooks.cpp (3)
- 453-462: The hook at
0x440D01for handling the unlimbo state ofDestroyableObstaclebuildings correctly callsRecalculateCellsto update the map state. This is a good use of the function and ensures that the game's pathfinding and other systems are aware of the changes in obstacle status. No specific issues here, but ensure thorough testing to catch any edge cases in map behavior changes.- 465-474: Similarly, the hook at
0x445D87for handling the limbo state ofDestroyableObstaclebuildings usesRecalculateCells<true>to remove the building's influence on the map. This complements the unlimbo hook well, ensuring that obstacles are correctly accounted for both when they become active and when they are removed. Again, thorough testing is recommended to ensure that these changes integrate smoothly with the game's other systems.- 477-488: The hook at
0x483D8Efor checking passability related toDestroyableObstaclebuildings is straightforward and effectively uses theIsDestroyableObstacleproperty to determine if a building should be considered a blockage. This is a critical piece of the improved blockage handling logic, ensuring that units' pathfinding respects the destructible nature of certain obstacles. No specific issues, but as with the other hooks, testing for unexpected interactions with game mechanics is advisable.src/Ext/Techno/Body.cpp (2)
- 5-5: Including
OverlayTypeClass.his necessary for the new pathfinding logic that involves interactions with wall overlays. This change seems appropriate for the described feature.- 9-9: Including
Ext/WeaponType/Body.his required for accessing extended weapon type properties, which is crucial for the new pathfinding logic that considers weapon capabilities against blockages.CREDITS.md (1)
- 222-222: The addition to the credits for including functionality to consider buildings as destroyable pathfinding obstacles is a good practice. Acknowledging contributions is important for open-source projects, and this change properly credits the work done to improve the game's pathfinding logic.
src/Ext/Techno/Hooks.Firing.cpp (2)
- 50-66: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [40-54]
The renaming of
SecondarytoReturnWeaponIndexand the update of return values in theTechnoClass_WhatWeaponShouldIUse_ForceFirefunction improve readability and clarity. However, ensure that all references to this variable across the codebase are updated to reflect this change.Verification successful
The usage of
ReturnWeaponIndexis confined tosrc/Ext/Techno/Hooks.Firing.cpp, with no indications of broader usage across the codebase. This suggests that the concern for updating references across the codebase may not apply in this context. Therefore, the review comment appears to be overly cautious without necessity.* 816-847: The addition of hooks for wall weapon handling in `TechnoClass_WhatAction_WallWeapon`, `InfantryClass_CanEnterCell_WallWeapon`, and `UnitClass_CanEnterCell_WallWeapon` introduces a more nuanced approach to determining actions and entry capabilities based on weapon indices against walls. Ensure that the `TechnoExt::GetWeaponIndexAgainstWall` function is thoroughly tested for various overlay types and techno classes to prevent unexpected behavior.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that ReturnWeaponIndex is consistently used across the project. rg "ReturnWeaponIndex" --type cppLength of output: 230
Verification successful
Given the limitations, let's proceed to verify the existence and usage of the
TechnoExt::GetWeaponIndexAgainstWallfunction within the codebase, and how it's integrated within the specified hooks. We'll also look for any comments or documentation related to its testing or expected behavior.
The review comment is accurate in highlighting the addition of hooks for wall weapon handling and the use of the
TechnoExt::GetWeaponIndexAgainstWallfunction within those hooks. The function's existence and its application in the specified locations within the codebase have been confirmed. While the recommendation for thorough testing across various overlay types and techno classes cannot be directly verified here, it remains an essential consideration for ensuring the implementation's reliability and effectiveness.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Test the new wall weapon handling logic with various overlay types and techno classes. # Placeholder for specific test commands or scripts. echo "Run comprehensive tests for wall weapon handling."Length of output: 108
Script:
#!/bin/bash # Verify the existence of TechnoExt::GetWeaponIndexAgainstWall function rg "GetWeaponIndexAgainstWall" --context 5 # Check for usage within the mentioned hooks rg "GetWeaponIndexAgainstWall" src/Ext/Techno/Hooks.Firing.cpp --context 5Length of output: 5780
docs/Whats-New.md (1)
- 359-359: The documentation clearly states the new feature regarding buildings being considered as destroyable pathfinding obstacles. This addition should significantly impact gameplay and mod development, offering more dynamic interactions with the environment. It's well-placed in the changelog under the "New" category for the upcoming version, ensuring visibility to modders and players looking forward to the new features.
docs/New-or-Enhanced-Logics.md (2)
- 310-318: The documentation for destroyable pathfinding obstacles is clear and concise. It correctly explains how to make buildings be considered as destroyable obstacles in pathfinding by setting
IsDestroyableBlockageto true. This feature could significantly impact gameplay by allowing units to clear paths by destroying certain buildings, enhancing strategic options.- 307-322: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [320-325]
The power plant enhancer feature introduces an interesting gameplay mechanic, allowing for the increase of power generation through specific enhancements. This could add depth to resource management strategies. However, it's important to ensure that this feature is balanced to prevent it from becoming overpowered or undermining the importance of building additional power plants.
| // Gets weapon index for a weapon to use against wall overlay. | ||
| int TechnoExt::GetWeaponIndexAgainstWall(TechnoClass* pThis, OverlayTypeClass* pWallOverlayType) | ||
| { | ||
| auto const pTechnoType = pThis->GetTechnoType(); | ||
| int weaponIndex = -1; | ||
| auto pWeapon = TechnoExt::GetCurrentWeapon(pThis, weaponIndex); | ||
|
|
||
| if ((pTechnoType->TurretCount > 0 && !pTechnoType->IsGattling) || !pWallOverlayType) | ||
| return weaponIndex; | ||
| else if (weaponIndex == -1) | ||
| return 0; | ||
|
|
||
| if (!pWeapon || (!pWeapon->Warhead->Wall && (!pWeapon->Warhead->Wood || pWallOverlayType->Armor != Armor::Wood))) | ||
| { | ||
| int weaponIndexSec = -1; | ||
| pWeapon = TechnoExt::GetCurrentWeapon(pThis, weaponIndexSec, true); | ||
|
|
||
| if (pWeapon && (pWeapon->Warhead->Wall || (pWeapon->Warhead->Wood && pWallOverlayType->Armor == Armor::Wood) | ||
| && !TechnoTypeExt::ExtMap.Find(pTechnoType)->NoSecondaryWeaponFallback)) | ||
| { | ||
| return weaponIndexSec; | ||
| } | ||
|
|
||
| return weaponIndex; | ||
| } | ||
|
|
||
| return weaponIndex; | ||
| } |
There was a problem hiding this comment.
The GetWeaponIndexAgainstWall function introduces logic to select the appropriate weapon index when a unit encounters a wall overlay during pathfinding. This function checks for the presence of a turret, the type of wall overlay, and whether the weapon's warhead is effective against walls or wood, depending on the overlay's armor type. The logic appears sound and aligns with the PR's objective to enhance pathfinding by considering weapon capabilities. However, there are a few points to consider for improvement:
- The condition in line 374 might be overly restrictive by only allowing units without gattling weapons and with turrets to use their primary weapon against walls. This could limit the functionality for units that might have effective secondary weapons against walls. Consider whether this behavior is intended or if it should be more flexible.
- The fallback logic to check for a secondary weapon (lines 381-390) is a good approach, but it's important to ensure that
NoSecondaryWeaponFallbackis correctly checked to prevent unintended behavior. - The function returns
-1if no suitable weapon is found, which is standard, but ensure that the calling code handles this return value appropriately to avoid unexpected behavior.
Overall, the function aligns with the PR's goals, but a few clarifications and potential adjustments could enhance its robustness and flexibility.
- if ((pTechnoType->TurretCount > 0 && !pTechnoType->IsGattling) || !pWallOverlayType)
+ if (!pWallOverlayType)Consider removing the turret and gattling checks to allow more flexibility in weapon selection against walls. Ensure that the calling code and game logic can handle units using secondary weapons when appropriate.
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.
| // Gets weapon index for a weapon to use against wall overlay. | |
| int TechnoExt::GetWeaponIndexAgainstWall(TechnoClass* pThis, OverlayTypeClass* pWallOverlayType) | |
| { | |
| auto const pTechnoType = pThis->GetTechnoType(); | |
| int weaponIndex = -1; | |
| auto pWeapon = TechnoExt::GetCurrentWeapon(pThis, weaponIndex); | |
| if ((pTechnoType->TurretCount > 0 && !pTechnoType->IsGattling) || !pWallOverlayType) | |
| return weaponIndex; | |
| else if (weaponIndex == -1) | |
| return 0; | |
| if (!pWeapon || (!pWeapon->Warhead->Wall && (!pWeapon->Warhead->Wood || pWallOverlayType->Armor != Armor::Wood))) | |
| { | |
| int weaponIndexSec = -1; | |
| pWeapon = TechnoExt::GetCurrentWeapon(pThis, weaponIndexSec, true); | |
| if (pWeapon && (pWeapon->Warhead->Wall || (pWeapon->Warhead->Wood && pWallOverlayType->Armor == Armor::Wood) | |
| && !TechnoTypeExt::ExtMap.Find(pTechnoType)->NoSecondaryWeaponFallback)) | |
| { | |
| return weaponIndexSec; | |
| } | |
| return weaponIndex; | |
| } | |
| return weaponIndex; | |
| } | |
| // Gets weapon index for a weapon to use against wall overlay. | |
| int TechnoExt::GetWeaponIndexAgainstWall(TechnoClass* pThis, OverlayTypeClass* pWallOverlayType) | |
| { | |
| auto const pTechnoType = pThis->GetTechnoType(); | |
| int weaponIndex = -1; | |
| auto pWeapon = TechnoExt::GetCurrentWeapon(pThis, weaponIndex); | |
| if (!pWallOverlayType) | |
| return weaponIndex; | |
| else if (weaponIndex == -1) | |
| return 0; | |
| if (!pWeapon || (!pWeapon->Warhead->Wall && (!pWeapon->Warhead->Wood || pWallOverlayType->Armor != Armor::Wood))) | |
| { | |
| int weaponIndexSec = -1; | |
| pWeapon = TechnoExt::GetCurrentWeapon(pThis, weaponIndexSec, true); | |
| if (pWeapon && (pWeapon->Warhead->Wall || (pWeapon->Warhead->Wood && pWallOverlayType->Armor == Armor::Wood) | |
| && !TechnoTypeExt::ExtMap.Find(pTechnoType)->NoSecondaryWeaponFallback)) | |
| { | |
| return weaponIndexSec; | |
| } | |
| return weaponIndex; | |
| } | |
| return weaponIndex; | |
| } |
| // Get all cells covered by the building, optionally including those covered by OccupyHeight. | ||
| const std::vector<CellStruct> BuildingExt::GetFoundationCells(BuildingClass* const pThis, CellStruct const baseCoords, bool includeOccupyHeight) | ||
| { | ||
| const CellStruct foundationEnd = { 0x7FFF, 0x7FFF }; | ||
| auto const pFoundation = pThis->GetFoundationData(false); | ||
|
|
||
| int occupyHeight = includeOccupyHeight ? pThis->Type->OccupyHeight : 1; | ||
|
|
||
| if (occupyHeight <= 0) | ||
| occupyHeight = 1; | ||
|
|
||
| auto pCellIterator = pFoundation; | ||
|
|
||
| while (*pCellIterator != foundationEnd) | ||
| ++pCellIterator; | ||
|
|
||
| std::vector<CellStruct> foundationCells; | ||
| foundationCells.reserve(static_cast<int>(std::distance(pFoundation, pCellIterator + 1)) * occupyHeight); | ||
| pCellIterator = pFoundation; | ||
|
|
||
| while (*pCellIterator != foundationEnd) | ||
| { | ||
| auto actualCell = baseCoords + *pCellIterator; | ||
|
|
||
| for (auto i = occupyHeight; i > 0; --i) | ||
| { | ||
| foundationCells.push_back(actualCell); | ||
| --actualCell.X; | ||
| --actualCell.Y; | ||
| } | ||
| ++pCellIterator; | ||
| } | ||
|
|
||
| std::sort(foundationCells.begin(), foundationCells.end(), | ||
| [](const CellStruct& lhs, const CellStruct& rhs) -> bool | ||
| { | ||
| return lhs.X > rhs.X || lhs.X == rhs.X && lhs.Y > rhs.Y; | ||
| }); | ||
|
|
||
| auto const it = std::unique(foundationCells.begin(), foundationCells.end()); | ||
| foundationCells.erase(it, foundationCells.end()); | ||
|
|
||
| return foundationCells; | ||
| } |
There was a problem hiding this comment.
The implementation of GetFoundationCells function seems logically sound and correctly calculates the cells covered by a building. However, there are a few areas for potential improvement:
-
Performance: The function iterates over the foundation data twice - first to calculate the size for
reserveand then to actually populatefoundationCells. This could be optimized by calculating the size directly from the building's foundation dimensions, if such metadata is available, reducing the need for the first iteration. -
Complexity: The function's logic, especially the double iteration over foundation data and the manipulation of
actualCellwithin the loop, could be simplified for readability and maintainability. Consider breaking down the function into smaller, more focused sub-functions if possible. -
Sorting and uniqueness: The sorting and removal of duplicate cells (
std::unique) at the end of the function is necessary but could be a performance concern for buildings with large foundations. If the order of cells is not critical for the callers of this function, consider using astd::setorstd::unordered_setfrom the beginning to ensure uniqueness without the need for sorting and manual deduplication.
Overall, the function achieves its goal, but reviewing these aspects could lead to improved performance and maintainability.
| template <bool remove = false> | ||
| static void RecalculateCells(BuildingClass* pThis) | ||
| { | ||
| auto const cells = BuildingExt::GetFoundationCells(pThis, pThis->GetMapCoords()); | ||
|
|
||
| auto& map = MapClass::Instance; | ||
|
|
||
| for (auto const& cell : cells) | ||
| { | ||
| if (auto pCell = map->TryGetCellAt(cell)) | ||
| { | ||
| pCell->RecalcAttributes(DWORD(-1)); | ||
|
|
||
| if constexpr (remove) | ||
| map->ResetZones(cell); | ||
| else | ||
| map->RecalculateZones(cell); | ||
|
|
||
| map->RecalculateSubZones(cell); | ||
|
|
||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The RecalculateCells template function is well-implemented and serves its purpose of recalculating cell attributes and zones for buildings marked as DestroyableObstacle. A few considerations:
-
Template usage: The use of a template parameter
removeto switch behavior between adding and removing obstacles is clever but might obscure the function's intent at the call sites. Consider if separate, clearly named functions would improve readability and maintainability. -
Efficiency: The function iterates over all foundation cells to recalculate attributes and zones. Ensure that this operation is not overly costly in terms of performance, especially for large buildings or frequent state changes between limbo and unlimbo.
-
Error handling: While the function checks for null pointers when accessing cells (
if (auto pCell = map->TryGetCellAt(cell))), consider logging or handling unexpected scenarios where cells expected to be part of a building's foundation are not found on the map.
Overall, the function and its usage in the hooks are logically sound, but refining based on these points could enhance clarity and robustness.
| @@ -102,4 +102,5 @@ class BuildingExt | |||
| static bool DoGrindingExtras(BuildingClass* pBuilding, TechnoClass* pTechno, int refund); | |||
| static bool HandleInfiltrate(BuildingClass* pBuilding, HouseClass* pInfiltratorHouse); | |||
| static bool CanUndeployOnSell(BuildingClass* pThis); | |||
| static const std::vector<CellStruct> GetFoundationCells(BuildingClass* pThis, CellStruct baseCoords, bool includeOccupyHeight = false); | |||
There was a problem hiding this comment.
The addition of GetFoundationCells as a static method in BuildingExt class is a good practice for utility functions that might be used across the project. Ensure that its implementation efficiently calculates the foundation cells, considering the includeOccupyHeight parameter for different use cases. It would also be beneficial to add documentation comments explaining the method's purpose, parameters, and return value for better maintainability.
| @@ -60,6 +60,7 @@ class BuildingTypeExt | |||
| Nullable<bool> ConsideredVehicle; | |||
| Valueable<bool> ZShapePointMove_OnBuildup; | |||
| Valueable<int> SellBuildupLength; | |||
| Valueable<bool> IsDestroyableObstacle; | |||
There was a problem hiding this comment.
The addition of IsDestroyableObstacle to BuildingTypeExt::ExtData with a default value of false is a thoughtful approach to introducing new behavior while maintaining backward compatibility. This change likely supports the PR's enhancements to pathfinding and blockage handling. It's important to ensure that all relevant documentation is updated to include this new property and its usage within the project.
| @@ -143,6 +143,7 @@ class TechnoExt | |||
| static bool ConvertToType(FootClass* pThis, TechnoTypeClass* toType); | |||
| static bool CanDeployIntoBuilding(UnitClass* pThis, bool noDeploysIntoDefaultValue = false); | |||
| static bool IsTypeImmune(TechnoClass* pThis, TechnoClass* pSource); | |||
| static int GetWeaponIndexAgainstWall(TechnoClass* pThis, OverlayTypeClass* pWallOverlayType); | |||
There was a problem hiding this comment.
The introduction of GetWeaponIndexAgainstWall in the TechnoExt class suggests an enhancement in how units interact with wall obstacles, likely related to the improved pathfinding features. It's crucial that the implementation of this method accurately determines the best weapon for the given wall overlay type. Adding documentation for this method would also improve code maintainability by explaining its purpose, parameters, and expected behavior.
| @@ -146,6 +146,7 @@ void BuildingTypeExt::ExtData::LoadFromINIFile(CCINIClass* const pINI) | |||
|
|
|||
| this->ConsideredVehicle.Read(exINI, pSection, "ConsideredVehicle"); | |||
| this->SellBuildupLength.Read(exINI, pSection, "SellBuildupLength"); | |||
| this->IsDestroyableObstacle.Read(exINI, pSection, "IsDestroyableObstacle"); | |||
There was a problem hiding this comment.
Proper handling of the IsDestroyableObstacle property in the LoadFromINIFile method allows for flexible game customization through INI configurations. This is a positive addition, enhancing the game's moddability. Ensure that the documentation for INI files is updated to include this new property.
| @@ -234,6 +235,7 @@ void BuildingTypeExt::ExtData::Serialize(T& Stm) | |||
| .Process(this->ConsideredVehicle) | |||
| .Process(this->ZShapePointMove_OnBuildup) | |||
| .Process(this->SellBuildupLength) | |||
| .Process(this->IsDestroyableObstacle) | |||
There was a problem hiding this comment.
Correct serialization of the IsDestroyableObstacle property ensures consistency and reliability in saving and loading game states. This addition is crucial for maintaining the integrity of game data across sessions.
| if (pCell->OverlayData >> 4 != pOverlayType->DamageLevels) | ||
| { | ||
| return Secondary; | ||
| R->EAX(TechnoExt::GetWeaponIndexAgainstWall(pThis, pOverlayType)); | ||
| return ReturnWeaponIndex; |
There was a problem hiding this comment.
The logic for handling overlay types in TechnoClass_WhatWeaponShouldIUse_ForceFire is sound, but consider adding comments to explain the significance of pCell->OverlayData >> 4 != pOverlayType->DamageLevels for future maintainability.
// Check if the overlay's damage level does not match the expected value, indicating a potential target for wall weapon usage.
if (pCell->OverlayData >> 4 != pOverlayType->DamageLevels)| DEFINE_HOOK(0x73F495, UnitClass_CanEnterCell_WallWeapon, 0x6) | ||
| { | ||
| GET(TechnoClass*, pThis, ESI); | ||
| enum { SkipGameCode = 0x73F4A1 }; | ||
|
|
||
| R->EAX(TechnoClass_GetWeaponAgainstWallWrapper(pThis, nullptr, 0)); | ||
| GET(UnitClass*, pThis, EBX); | ||
| GET(OverlayTypeClass*, pOverlayTypeClass, ESI); | ||
|
|
||
| return 0; | ||
| R->EAX(pThis->GetWeapon(TechnoExt::GetWeaponIndexAgainstWall(pThis, pOverlayTypeClass))); | ||
|
|
||
| return SkipGameCode; |
There was a problem hiding this comment.
In the UnitClass_CanEnterCell_WallWeapon hook, the logic correctly determines the weapon index against walls for unit classes. This is a crucial enhancement for gameplay, as it allows units to interact with walls based on their weapon capabilities. To ensure robustness, consider adding unit tests covering edge cases, such as units with no weapons or special wall interactions.
Would you like me to help with creating unit tests for these scenarios?
|
The pathfinding 'improvements' failed to actually achieve what they were supposed to and in fact were counterproductive since in many cases they resulted in pathfinding failures when there were too many impassable objects around. It is probably better to leave units that are incapable of firing against buildings to try and go to closest possible cell than fail to move entirely. That said, this PR now consists (besides the wall weapon selection cleanup I decided to keep in) of a feature that toggles buildings to serve as destroyable obstacles akin to walls or TerrainTypes that are only considered 'passable' for units with appropriate |
Starkku
force-pushed
the
feature/blockage-pathfinding
branch
from
e9de76f
to
25dfdc8
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (14)
- CREDITS.md (1 hunks)
- YRpp (1 hunks)
- docs/New-or-Enhanced-Logics.md (1 hunks)
- docs/Whats-New.md (1 hunks)
- src/Ext/Building/Body.cpp (1 hunks)
- src/Ext/Building/Body.h (1 hunks)
- src/Ext/Building/Hooks.cpp (1 hunks)
- src/Ext/BuildingType/Body.cpp (2 hunks)
- src/Ext/BuildingType/Body.h (2 hunks)
- src/Ext/Techno/Body.cpp (2 hunks)
- src/Ext/Techno/Body.h (1 hunks)
- src/Ext/Techno/Hooks.Firing.cpp (3 hunks)
- src/Ext/Techno/Hooks.cpp (1 hunks)
- src/Ext/TerrainType/Hooks.Passable.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/Ext/Techno/Hooks.cpp
Files skipped from review as they are similar to previous changes (12)
- CREDITS.md
- YRpp
- docs/New-or-Enhanced-Logics.md
- docs/Whats-New.md
- src/Ext/Building/Body.cpp
- src/Ext/Building/Body.h
- src/Ext/Building/Hooks.cpp
- src/Ext/BuildingType/Body.cpp
- src/Ext/BuildingType/Body.h
- src/Ext/Techno/Body.cpp
- src/Ext/Techno/Body.h
- src/Ext/Techno/Hooks.Firing.cpp
| DEFINE_HOOK(0x73FBA7, UnitClass_CanEnterCell_PassableTerrain, 0x5) | ||
| { | ||
| enum { ReturnPassable = 0x73FD37, SkipTerrainChecks = 0x73FA7C }; | ||
|
|
||
| GET(AbstractClass*, pTarget, ESI); | ||
| GET(TerrainClass*, pTerrain, ESI); | ||
|
|
||
| if (auto const pTerrain = abstract_cast<TerrainClass*>(pTarget)) | ||
| { | ||
| auto const pTypeExt = TerrainTypeExt::ExtMap.Find(pTerrain->Type); | ||
| auto const pTypeExt = TerrainTypeExt::ExtMap.Find(pTerrain->Type); | ||
|
|
||
| if (pTypeExt->IsPassable) | ||
| { | ||
| if (IS_CELL_OCCUPIED(pTerrain->GetCell())) | ||
| return SkipTerrainChecks; | ||
| if (pTypeExt->IsPassable) | ||
| { | ||
| if (IS_CELL_OCCUPIED(pTerrain->GetCell())) | ||
| return SkipTerrainChecks; | ||
|
|
||
| R->EBP(0); | ||
| return ReturnPassable; | ||
| } | ||
| R->EBP(0); | ||
| return ReturnPassable; |
There was a problem hiding this comment.
The logic within UnitClass_CanEnterCell_PassableTerrain correctly checks if the terrain type is passable and if the cell is occupied. However, ensure that pTypeExt->IsPassable is always valid before dereferencing it to avoid potential null pointer dereferences. This is a follow-up on a previous comment, and it seems the issue might still be relevant if TerrainTypeExt::ExtMap.Find could potentially return nullptr for some terrain types. Consider adding a null check for pTypeExt before accessing IsPassable.
- auto const pTypeExt = TerrainTypeExt::ExtMap.Find(pTerrain->Type);
+ auto const pTypeExt = TerrainTypeExt::ExtMap.Find(pTerrain->Type);
+ if (!pTypeExt) return 0; // Skip if terrain type extension is not foundCommittable 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.
| DEFINE_HOOK(0x73FBA7, UnitClass_CanEnterCell_PassableTerrain, 0x5) | |
| { | |
| enum { ReturnPassable = 0x73FD37, SkipTerrainChecks = 0x73FA7C }; | |
| GET(AbstractClass*, pTarget, ESI); | |
| GET(TerrainClass*, pTerrain, ESI); | |
| if (auto const pTerrain = abstract_cast<TerrainClass*>(pTarget)) | |
| { | |
| auto const pTypeExt = TerrainTypeExt::ExtMap.Find(pTerrain->Type); | |
| auto const pTypeExt = TerrainTypeExt::ExtMap.Find(pTerrain->Type); | |
| if (pTypeExt->IsPassable) | |
| { | |
| if (IS_CELL_OCCUPIED(pTerrain->GetCell())) | |
| return SkipTerrainChecks; | |
| if (pTypeExt->IsPassable) | |
| { | |
| if (IS_CELL_OCCUPIED(pTerrain->GetCell())) | |
| return SkipTerrainChecks; | |
| R->EBP(0); | |
| return ReturnPassable; | |
| } | |
| R->EBP(0); | |
| return ReturnPassable; | |
| DEFINE_HOOK(0x73FBA7, UnitClass_CanEnterCell_PassableTerrain, 0x5) | |
| { | |
| enum { ReturnPassable = 0x73FD37, SkipTerrainChecks = 0x73FA7C }; | |
| GET(TerrainClass*, pTerrain, ESI); | |
| auto const pTypeExt = TerrainTypeExt::ExtMap.Find(pTerrain->Type); | |
| if (!pTypeExt) return 0; // Skip if terrain type extension is not found | |
| if (pTypeExt->IsPassable) | |
| { | |
| if (IS_CELL_OCCUPIED(pTerrain->GetCell())) | |
| return SkipTerrainChecks; | |
| R->EBP(0); | |
| return ReturnPassable; |
Starkku
force-pushed
the
feature/blockage-pathfinding
branch
from
84a919e
to
c192350
Compare
Starkku
force-pushed
the
feature/blockage-pathfinding
branch
from
c192350
to
593b200
Compare
Starkku
force-pushed
the
feature/blockage-pathfinding
branch
from
593b200
to
1b95e54
Compare
Starkku
force-pushed
the
feature/blockage-pathfinding
branch
from
1b95e54
to
6996f86
Compare
Starkku
force-pushed
the
feature/blockage-pathfinding
branch
from
6996f86
to
c5fa162
Compare
- Fix overridden instructions causing infantry to treat TerrainTypes as impassable - Fix conflict with passable TerrainType hook potentially causing issues - Cleaned up wall weapon selection / check code
Starkku
force-pushed
the
feature/blockage-pathfinding
branch
from
c5fa162
to
926a02a
Compare
Destroyable pathfinding obstacles
IsDestroyableBlockageto true. What this does is make the building be considered impassable and impenetrable pathfinding obstacle to every unit that is not flying or have appropriateMovementZone(ones that allow destroyable obstacles to be overcome, e.g(Infantry|Amphibious)Destroyer) akin to wall overlays and TerrainTypes.MovementZonebut no means to actually destroy an obstacle (such as a weapon that can fire and deal damage at them), they will get stuck trying to go through them instead of pathing around.In
rulesmd.ini:In addition there is some cleanup to how technos select a weapon to use against walls.
Summary by CodeRabbit