Update #1
Update #1
Conversation
ChrisLv-CN
force-pushed
the
phobos-dev
branch
from
e42c20d to
bb80345
Compare
ChrisLv-CN
force-pushed
the
phobos-dev
branch
from
b7a1a55 to
7630681
Compare
📝 WalkthroughWalkthroughAdds an overload to HouseClass::FindSuperWeapon, renames a HouseClass public field to PowerSurplus, updates TechnoClass public API/signatures and adds multiple targeting/fire helper methods, and refactors casting helpers in Helpers/Cast.h to support a SkipNullCheck template parameter and macro-generated specializations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 11
Configuration used: CodeRabbit UI
Files selected for processing (18)
- BasicStructures.h (3 hunks)
- CCINIClass.h (1 hunks)
- CellClass.h (3 hunks)
- Dir.h (5 hunks)
- DisplayClass.h (2 hunks)
- EBolt.h (1 hunks)
- Facing.h (2 hunks)
- HouseClass.h (2 hunks)
- Kamikaze.h (1 hunks)
- MessageListClass.h (1 hunks)
- ObjectClass.h (2 hunks)
- SpawnManagerClass.h (1 hunks)
- StaticInits.cpp (1 hunks)
- Surface.h (7 hunks)
- TechnoClass.h (7 hunks)
- TechnoTypeClass.h (1 hunks)
- WeaponTypeClass.h (1 hunks)
- YRMathVector.h (7 hunks)
Additional comments: 28
Kamikaze.h (1)
- 16-16: Change to
AbstractClass*forCellmember inKamikazeControlstruct is approved. Enhances flexibility in target handling for kamikaze missions.EBolt.h (2)
- 8-8: Addition of include directive for
ArrayClasses.his approved. Prepares file for enhanced functionality.- 10-10: Declaration of
TechnoClassis approved. Indicates interaction with game entities for electric bolts.SpawnManagerClass.h (1)
- 105-106: Renaming of members in
SpawnManagerClassimproves clarity.TargettoDestinationandNewTargettoTargetmake their roles more intuitive.Facing.h (1)
- 28-28: Removal of
explicitspecifier fromFacingClassconstructor and addition of clarifying comments are approved. Enhances usability and code clarity.WeaponTypeClass.h (1)
- 41-52: Addition of new methods for calculating speed and a static method for speed calculation in
WeaponTypeClassis approved. Enhances flexibility and accuracy of weapon interactions.MessageListClass.h (1)
- 86-88: Addition of a new
PrintMessagemethod with apLabelparameter and adjustments to existing methods inMessageListClassis approved. Improves messaging system flexibility and UI.BasicStructures.h (3)
- 44-48: Addition of a boolean operator to
ColorStructis approved. Simplifies truthiness checks.- 53-62: Introduction of a
Colorsnamespace with predefined color constants is approved. Enhances usability and readability.- 176-179: Addition of an
IsEmpty()method toRectangleStructis approved. Provides a straightforward way to check for default-initialized instances.Dir.h (2)
- 14-15: Refinement of
DirStructconstructors and methods inDir.his approved. Improves precision and flexibility of direction handling.- 55-72: Introduction of new functions for value manipulation in
DirStructis approved. Provides additional tools for working with direction values.DisplayClass.h (1)
- 7-20: Addition of new declarations and methods related to proximity checks and marking foundations in
DisplayClassis approved. Enhances building placement and interaction mechanics.StaticInits.cpp (1)
- 119-127: Addition of
HouseClass::FindSuperWeapon(SuperWeaponTypeClass* const type)method and bug fix inIsIonCannonEligibleTargetmethod inStaticInits.cppis approved. Enhances super weapon handling and target eligibility checks.YRMathVector.h (3)
- 19-24: Addition of new assignment operators for type conversions in
Vector2DandVector3Dclasses is approved. Enhances usability and functionality.- 81-85: Addition of comparison operators for map keys in
Vector2DandVector3Dclasses is approved. Allows use as map keys.- 125-129: Addition of an
IsEmptymethod toVector2DandVector3Dclasses is approved. Provides a straightforward way to check for empty vectors.CellClass.h (2)
- 27-49: The
TileTypeenum introduces hardcoded values for tile types. Ensure these values match expected game engine or documentation values, as hardcoded values can lead to maintenance issues if the underlying assumptions change.- 221-227: The overloaded
IsClearToMovemethod introduces default parameter values and calls the other overloaded version internally. Verify that the default value forlevel(-1) and the logic to determineisBridgeusingContainsBridge()align with the game's movement and terrain rules.TechnoClass.h (7)
- 26-26: The addition of
class EBoltsuggests new functionality related to electric attacks or effects. Ensure that its implementation is fully integrated with the rest of the TechnoClass methods and that it adheres to the game's mechanics for electric-based abilities or attacks.- 191-197: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [194-218]
The method signatures for
TurretFacingandGetRealFacinghave been changed to useFacingClass*instead ofDirStruct*. Confirm that all calls to these methods throughout the codebase have been updated to reflect this change, and verify thatFacingClassprovides the necessary functionality thatDirStructpreviously offered.
- 354-360: The method
Fire_IgnoreTypesuggests a specific behavior for firing mechanics, potentially overriding default type checks. Ensure this behavior is documented and justified within the game's logic, especially regarding how it interacts with different target types and weapon systems.- 370-374: The
CreateLaserandElectric_Zapmethods introduce new attack or effect types. Verify these methods are correctly implemented according to the game's specifications for laser and electric attacks, including proper handling of target types, damage calculations, and visual effects.- 376-378: The
DrawALinkTomethod adds functionality for drawing visual links between points. Ensure this method is used consistently and appropriately within the game's visual effect system, and that it does not introduce performance issues when rendering multiple links in a scene.- 632-633: The addition of the
ROFTimerproperty and modification of theROFproperty suggest changes to the rate of fire mechanics. Confirm that these changes are consistent with the intended gameplay balance and that theROFTimeris correctly utilized in weapon firing logic.- 503-511: The non-virtual methods
TurretFacingandGetRealFacingprovide convenience wrappers for their virtual counterparts. Ensure that these methods are used in contexts where the facing direction is required without modifying the underlying object, and that they correctly handle all possible states of turret and body facing.HouseClass.h (2)
- 14-14: Addition of
#include <SuperWeaponTypeClass.h>is appropriate for the new method signature that usesSuperWeaponTypeClass*. Ensure this new dependency is justified and used elsewhere in the file if possible to avoid unnecessary includes.- 508-509: Modification of
FindSuperWeaponmethod to take aSuperWeaponTypeClass*instead ofSuperWeaponTypeimproves type safety and clarity. Verify that all calls to this method have been updated accordingly across the codebase.
| class NOVTABLE Surface | ||
| { | ||
| public: | ||
| static constexpr reference<bool*, 0x84310Cu> const Pattern{}; |
There was a problem hiding this comment.
The Pattern constant is added to the Surface class. Ensure its usage is properly documented and consider if access modifiers (e.g., public, protected) are appropriate for its intended use.
| static bool __fastcall ClipLine(Point2D& point1, Point2D& point2, RectangleStruct& Bounds) | ||
| { JMP_STD(0x7BC2B0); } |
There was a problem hiding this comment.
The ClipLine method is added to the Surface class. Verify the correctness of the JMP_STD macro usage and ensure the method's parameters and return type align with its implementation requirements.
| bool DrawDashedLine(Point2D* pStart, Point2D* pEnd, int nColor, int nOffset) | ||
| { | ||
| return DrawDashedLine(pStart, pEnd, nColor, Pattern.get(), nOffset); | ||
| } |
There was a problem hiding this comment.
The DrawDashedLine method is added to the Surface class. Confirm that the Pattern constant is correctly utilized here and check for potential off-by-one errors in the handling of the nOffset parameter.
| RectangleStruct GetRect() | ||
| { | ||
| RectangleStruct ret; | ||
| RectangleStruct ret{ 0, 0, 0, 0 }; |
There was a problem hiding this comment.
The initialization of ret in the GetRect method of the Surface class has been modified. Ensure that the new initialization values are correct and consider if they might affect existing functionality.
| void DrawSHP(ConvertClass* Palette, SHPStruct* SHP, int FrameIndex, | ||
| const Point2D* const Position, const RectangleStruct* const Bounds, | ||
| BlitterFlags Flags = (BlitterFlags)0x600, | ||
| int Brightness = 1000, | ||
| int TintColor = 0) | ||
| { | ||
| DrawSHP(Palette, SHP, FrameIndex, Position, Bounds, Flags, 0, | ||
| 0, | ||
| ZGradient::Ground, | ||
| Brightness, | ||
| TintColor, nullptr, 0, 0, 0 | ||
| ); | ||
| } |
There was a problem hiding this comment.
The DrawSHP method overrides added to the DSurface class introduce new default parameter values. Confirm that these defaults are appropriate and do not inadvertently change the behavior of existing code that relies on this method.
| static constexpr reference<BYTE[], 0x826260u> const INI_Rules_FileName{}; | ||
| static constexpr reference<BYTE[], 0x82621Cu> const INI_AI_FileName{}; | ||
| static constexpr reference<BYTE[], 0x826254u> const INI_Art_FileName{}; |
There was a problem hiding this comment.
The declarations for INI_Rules_FileName, INI_AI_FileName, and INI_Art_FileName as static constexpr reference<BYTE[]> constants are added. Verify that the memory addresses these constants point to are correctly assigned and that the data types are appropriate for their usage.
| DamageState TakeDamage(int damage, WarheadTypeClass* pWH, bool crewed, bool ignoreDefenses = true, ObjectClass* pAttacker = nullptr, HouseClass* pAttackingHouse = nullptr) | ||
| { | ||
| return ReceiveDamage(&damage, 0, pWH, pAttacker, ignoreDefenses, !crewed, pAttackingHouse); | ||
| } |
There was a problem hiding this comment.
The first TakeDamage method added to ObjectClass includes parameters for specifying a WarheadTypeClass. Ensure that the method correctly forwards these parameters to ReceiveDamage and that the logic for handling crewed and ignoreDefenses flags is correct.
| DamageState TakeDamage(int damage, bool crewed, bool ignoreDefenses = true, ObjectClass* pAttacker = nullptr, HouseClass* pAttackingHouse = nullptr) | ||
| { | ||
| return TakeDamage(damage, RulesClass::Instance->C4Warhead, crewed, ignoreDefenses, pAttacker, pAttackingHouse); | ||
| } |
There was a problem hiding this comment.
The second TakeDamage method simplifies damage application by using a default warhead type from RulesClass. Confirm that the default warhead type is appropriate for all use cases where this method might be called and that the forwarding of parameters to the first TakeDamage method is correct.
| int GuardRange; | ||
| int MinDebris; | ||
| int MaxDebris; | ||
| int MinDebris; |
There was a problem hiding this comment.
The order of MinDebris and MaxDebris variables in TechnoTypeClass has been altered. Verify that this change does not affect serialization or memory layout in a way that could lead to compatibility issues with saved games or external tools that rely on the class's structure.
| TileType GetTileType() | ||
| { | ||
| if (Tile_Is_Tunnel()) return TileType::Tunnel; | ||
| if (Tile_Is_Water()) return TileType::Water; | ||
| if (Tile_Is_Blank()) return TileType::Blank; | ||
| if (Tile_Is_Ramp()) return TileType::Ramp; | ||
| if (Tile_Is_Cliff()) return TileType::Cliff; | ||
| if (Tile_Is_Shore()) return TileType::Shore; | ||
| if (Tile_Is_Wet()) return TileType::Wet; | ||
| if (Tile_Is_MiscPave()) return TileType::MiscPave; | ||
| if (Tile_Is_Pave()) return TileType::Pave; | ||
| if (Tile_Is_DirtRoad()) return TileType::DirtRoad; | ||
| if (Tile_Is_PavedRoad()) return TileType::PavedRoad; | ||
| if (Tile_Is_PavedRoadEnd()) return TileType::PavedRoadEnd; | ||
| if (Tile_Is_PavedRoadSlope()) return TileType::PavedRoadSlope; | ||
| if (Tile_Is_Median()) return TileType::Median; | ||
| if (Tile_Is_Bridge()) return TileType::Bridge; | ||
| if (Tile_Is_WoodBridge()) return TileType::WoodBridge; | ||
| if (Tile_Is_ClearToSandLAT()) return TileType::ClearToSandLAT; | ||
| if (Tile_Is_Green()) return TileType::Green; | ||
| if (Tile_Is_NotWater()) return TileType::NotWater; | ||
| if (Tile_Is_DestroyableCliff()) return TileType::DestroyableCliff; | ||
| return TileType::Unknown; | ||
| } |
There was a problem hiding this comment.
The GetTileType method uses a series of conditional checks to return the tile type. This approach is straightforward but consider if a more efficient mapping or querying mechanism is available, especially if this method is called frequently in performance-critical paths.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- BuildingClass.h (1 hunks)
Additional comments: 1
BuildingClass.h (1)
- 258-259: Changing
BStateandQueueBStatefrominttoBStateTypeimproves type safety and clarity. Ensure all usages of these variables are updated to handle the new enum type correctly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- Surface.h (7 hunks)
Additional comments: 9
Surface.h (9)
- 15-15: The addition of the
Patternconstant to theSurfaceclass is a good practice for defining reusable patterns. However, ensure that its usage is properly documented, especially since it's a public member. Consider adding comments to explain its purpose and how it should be used in drawing operations.- 113-114: The
ClipLinemethod uses theJMP_STDmacro for its implementation. Ensure that the macro correctly handles the logic for clipping lines within the specified bounds. It's also important to verify that the method's parameters (point1,point2, andBounds) are correctly used and that the method is tested thoroughly for edge cases.- 119-119: The modification in the initialization of
retin theGetRectmethod to{ 0, 0, 0, 0 }is a clear way to initialize structures. Ensure that this change does not affect any existing functionality whereGetRectis used, especially if previous default values were different or if uninitialized values were relied upon.- 124-127: The
DrawDashedLinemethod correctly utilizes thePatternconstant. Ensure that the method is thoroughly tested, particularly for the handling of thenOffsetparameter, to avoid potential off-by-one errors or incorrect pattern rendering.- 152-154: The addition of the
VoxelSurfaceconstant and the constructor forBSurfaceare well-implemented. Ensure that theBufferinitialization in the constructor takes into account the correct calculation for the buffer size based onWidth,Height, andBytesPerPixel. Also, verify that the virtual table address (0x7E2070) is correctly set forBSurfaceinstances.- 216-218: The
DrawLineBlitmethod in theDSurfaceclass is a good addition for drawing lines with blitting. Ensure that the parameters (pRect,pStart,pEnd,pStartColor,mult,start_z,end_z) are correctly used within the method and that the method is tested for various scenarios, including edge cases.- 232-244: The overloaded
DrawSHPmethod introduces new default parameter values. Confirm that these defaults (Flags = (BlitterFlags)0x600,Brightness = 1000,TintColor = 0) are appropriate and do not inadvertently change the behavior of existing code that relies on this method. It's crucial to ensure compatibility with all calls toDrawSHP.- 246-251: The second overloaded
DrawSHPmethod simplifies drawing by automatically determining the bounds. Ensure that the automatic bounds calculation (this->GetRect()) works correctly in all scenarios and does not introduce unexpected behavior, especially in edge cases where specific bounds are expected.- 229-257: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [254-266]
The
DrawTextmethods provide flexibility in drawing text with different parameters. Ensure that the default values forBackColorandFlagin the overloaded methods are sensible and that all variations ofDrawTextare tested for correct text rendering, especially in cases with different background colors and shadow types.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CellClass.h (3 hunks)
Additional comments: 2
CellClass.h (2)
- 27-49: The addition of the
TileTypeenum class introduces a structured way to represent different types of tiles within the game. This is a positive change as it enhances code readability and maintainability by replacing magic numbers or strings with a strongly-typed enumeration. However, it's important to ensure that the values assigned to each enum member (e.g.,Tunnel = 0x484AB0) are correct and meaningful within the context of the game's logic. If these values are intended to be memory addresses or function pointers (which seems unusual), this should be clearly documented to avoid confusion.- 328-351: The
GetTileTypemethod implementation is straightforward and aligns with the newly introducedTileTypeenum, providing a way to determine the tile type based on specific conditions. This method is a good example of encapsulating game logic related to tile types within theCellClass. However, as previously mentioned in the comments from the outdated commits, consider if there's a more efficient way to map or query these tile types, especially if this method is called frequently in performance-critical paths. If performance is not a concern or if the current approach is the most suitable given the game's architecture, then this implementation is acceptable.
| // pass | ||
| bool IsClearToMove(SpeedType speedType, bool ignoreInfantry, bool ignoreVehicles, ZoneType zone, MovementZone movementZone, int level, bool alt) | ||
| { JMP_THIS(0x4834A0); } | ||
|
|
||
| bool IsClearToMove(SpeedType speedType, MovementZone movementZone, bool ignoreInfantry = false, bool ignoreVehicles = false, int level = -1) | ||
| { | ||
| return IsClearToMove(speedType, ignoreInfantry, ignoreInfantry, ZoneType::None, movementZone, level, (bool)(this->Flags & CellFlags::CenterRevealed)); | ||
| } |
There was a problem hiding this comment.
The overloaded IsClearToMove functions provide flexibility in determining whether a cell is clear for movement based on various parameters. This is a useful addition for handling different movement scenarios. However, there's a potential issue in the second overload (lines 224-227) where ignoreInfantry is passed twice instead of passing ignoreVehicles as the third argument to the first overload. This looks like a copy-paste error and should be corrected to ensure the function behaves as intended.
- return IsClearToMove(speedType, ignoreInfantry, ignoreInfantry, ZoneType::None, movementZone, level, (bool)(this->Flags & CellFlags::CenterRevealed));
+ return IsClearToMove(speedType, ignoreInfantry, ignoreVehicles, ZoneType::None, movementZone, level, (bool)(this->Flags & CellFlags::CenterRevealed));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.
| // pass | |
| bool IsClearToMove(SpeedType speedType, bool ignoreInfantry, bool ignoreVehicles, ZoneType zone, MovementZone movementZone, int level, bool alt) | |
| { JMP_THIS(0x4834A0); } | |
| bool IsClearToMove(SpeedType speedType, MovementZone movementZone, bool ignoreInfantry = false, bool ignoreVehicles = false, int level = -1) | |
| { | |
| return IsClearToMove(speedType, ignoreInfantry, ignoreInfantry, ZoneType::None, movementZone, level, (bool)(this->Flags & CellFlags::CenterRevealed)); | |
| } | |
| // pass | |
| bool IsClearToMove(SpeedType speedType, bool ignoreInfantry, bool ignoreVehicles, ZoneType zone, MovementZone movementZone, int level, bool alt) | |
| { JMP_THIS(0x4834A0); } | |
| bool IsClearToMove(SpeedType speedType, MovementZone movementZone, bool ignoreInfantry = false, bool ignoreVehicles = false, int level = -1) | |
| { | |
| return IsClearToMove(speedType, ignoreInfantry, ignoreVehicles, ZoneType::None, movementZone, level, (bool)(this->Flags & CellFlags::CenterRevealed)); | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- AircraftTrackerClass.h (1 hunks)
Additional comments: 4
AircraftTrackerClass.h (4)
- 21-21: The use of
constant_ptrforInstanceis an interesting choice, ensuring type safety and const correctness. However, it's crucial to ensure that the memory address0x887888uis correctly mapped and does not lead to any potential issues such as memory leaks or access violations.- 24-24: The explicit declaration of a destructor with
RXis noted. Ensure that the destructor properly cleans up any dynamically allocated resources to prevent memory leaks. This is particularly important in a class that manages a collection of pointers.- 26-36: The methods
Add_Entry,Remove_Entry,Update_Entry, andSearch_Entryuse theJMP_THISmacro for their implementations. This approach is specific to hooking or patching compiled binaries and is not common in standard C++ development. Ensure that this technique is consistent with the project's coding standards and that it does not introduce any security or stability issues.- 47-48: The properties
TechnosandCurrentare declared usingDECLARE_PROPERTY. This macro likely simplifies the declaration of getters and setters for these properties. Ensure that the macro correctly handles memory management and thread safety, especially sinceDynamicVectorClass<TechnoClass*>involves pointers to objects that may be modified or deleted elsewhere in the code.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- AircraftClass.h (1 hunks)
Additional comments: 2
AircraftClass.h (2)
- 64-64: Renamed
unknown_bool_6C8toLoseAmmofor clarity.This renaming improves code readability by providing a meaningful name for the variable. Ensure that all references to this variable in the codebase have been updated accordingly.
- 70-72: Added
IsLocked,ParadropsLeft, andIsCarrayallboolean flags.These additions seem to enhance the functionality and state management of the
AircraftClass. It's important to ensure that these new flags are properly initialized in the class constructor and handled correctly throughout the codebase.
| int UpdateDeplory() | ||
| { JMP_THIS(0x521320); } |
There was a problem hiding this comment.
The method name UpdateDeplory appears to be a typo and should likely be UpdateDeploy for clarity and consistency.
- int UpdateDeplory()
+ int UpdateDeploy()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.
| int UpdateDeplory() | |
| { JMP_THIS(0x521320); } | |
| int UpdateDeploy() | |
| { JMP_THIS(0x521320); } |
| void DestroyVoxelShadowCache() | ||
| { JMP_STD(0x5F99E0); } | ||
|
|
There was a problem hiding this comment.
The addition of DestroyVoxelShadowCache() is a straightforward enhancement for managing the voxel shadow cache. It's a good practice to include a brief comment above the function declaration to explain its purpose and any potential side effects or considerations for its use, enhancing code maintainability and readability.
+ // Destroys the voxel shadow cache to manage memory and rendering performance.
void DestroyVoxelShadowCache()
{ JMP_STD(0x5F99E0); }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.
| void DestroyVoxelShadowCache() | |
| { JMP_STD(0x5F99E0); } | |
| // Destroys the voxel shadow cache to manage memory and rendering performance. | |
| void DestroyVoxelShadowCache() | |
| { JMP_STD(0x5F99E0); } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CellClass.h (5 hunks)
- JumpjetLocomotionClass.h (1 hunks)
- MissionClass.h (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- CellClass.h
Additional comments: 2
JumpjetLocomotionClass.h (1)
- 62-63: Consider adding documentation for the
GetZBalloonHovermethod to explain its purpose and how it integrates with the rest of the class's functionality. This will improve maintainability and understandability of the code.MissionClass.h (1)
- 92-93: Consider adding documentation for the
Mission_Move_Carryallfunction to explain its specific use case forAircraftClass. Additionally, implementing type safety or checks to ensure it's only used withAircraftClasscould prevent potential misuse and improve code safety.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- TacticalClass.h (1 hunks)
Additional comments: 1
TacticalClass.h (1)
- 36-40: The addition of the
CoordsToClientmethod is a good enhancement for simplifying the usage of converting coordinates to client space. It encapsulates the buffer handling internally, making the API cleaner for callers.However, consider adding a brief comment above the method to explain its purpose and usage, especially since it overloads an existing method. This will improve code readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- ShipLocomotionClass.h (1 hunks)
Additional Context Used
Additional comments not posted (1)
ShipLocomotionClass.h (1)
31-32: RenamingRamp1andRamp2toPreviousRampandCurrentRampimproves the semantic clarity of these variables. This change likely enhances the readability and maintainability of the code by making the purpose of these variables more explicit. Ensure that all references to these variables throughout the codebase have been updated accordingly to reflect the new names.Verification successful
The renaming of
Ramp1andRamp2toPreviousRampandCurrentRamphas been successfully verified in theShipLocomotionClass.hfile, and no references to the old variable names were found. Additionally, it was discovered that similar changes were applied to theDriveLocomotionClass.hfile, indicating a broader scope of the renaming effort than initially reviewed. This broader application of the changes should be noted, but it does not affect the approval of the changes made toShipLocomotionClass.h. All references to these variables throughout the codebase appear to have been updated accordingly to reflect the new names.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search to find files that might reference the old ramp variables. Expecting no results. rg --type cpp $'Ramp1|Ramp2' # Search new variable names to validate usage. Expecting list of locations where the new names are used. rg --type cpp $'PreviousRamp|CurrentRamp'Length of output: 244
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
HouseClass.h (1)
14-14: LGTM! Consider forward declaration as an optimization.The include is necessary for the new
FindSuperWeaponoverload. However, as an optional optimization, you could use a forward declaration instead of including the full header to reduce compilation dependencies, since you're only using a pointer type.♻️ Optional: Use forward declaration instead
If compilation time becomes a concern, you could forward declare instead of including:
-#include <SuperWeaponTypeClass.h>And add to the forward declarations section:
//forward declarations class AnimClass; class BulletClass; class CellClass; class FactoryClass; class ObjectClass; class SuperClass; +class SuperWeaponTypeClass; class TagClass; class WaypointPathClass; class WaypointClass;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
HouseClass.h
🔇 Additional comments (2)
HouseClass.h (2)
508-509: Implementation verified and properly handles null pointers.The new overload at StaticInits.cpp:119-127 correctly implements the declaration. When a null pointer is passed, the loop safely returns
nullptrwithout finding a match, which is the appropriate behavior.
1017-1017: No action required—the type change from DWORD to int is correct and consistent with the codebase.The power system throughout YRpp (PowerOutput, PowerDrain, and related fields in HouseClass, BuildingTypeClass, and PowerClass) consistently uses
int(signed 32-bit), not DWORD. PowerSurplus asintaligns with the existing design and supports the logical use case of representing power surplus/deficit (which can be negative). All power comparisons and calculations already assume signed integer semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
TechnoClass.h (2)
354-355: Consider parameter consistency, though likely intentional for binary compatibility.The first parameter
CoordStruct* pLocationuses a pointer, while other methods in this file typically passCoordStructby const reference (e.g., lines 262, 294, 370, 373). Since this is a thunk to existing game code, this inconsistency is likely required for binary compatibility with the original function at address 0x6F7220.
357-359: Fix typo and consider clarifying the comment.The comment has a typo: "diffrent" → "different". Additionally, the comment could be more precise about what "IgnoreType" means in the method name.
📝 Suggested improvement
- // Unit and Infantry fire logic is diffrent, this function for help to fire custom weapon, force use Unit's fire logic + // Unit and Infantry have different fire logic. This function forces the use of Unit's fire logic regardless of the actual type, useful for firing custom weapons. BulletClass* Fire_IgnoreType(AbstractClass* pTarget, int idxWeapon) { JMP_THIS(0x6FDD50); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
TechnoClass.h
🧰 Additional context used
🧬 Code graph analysis (1)
TechnoClass.h (2)
EBolt.h (1)
EBolt(13-61)Facing.h (3)
FacingClass(13-16)FacingClass(18-21)FacingClass(23-26)
🔇 Additional comments (8)
TechnoClass.h (8)
26-26: LGTM!The forward declaration is correctly added to support the new
Electric_Zapmethod introduced later in the file.
509-520: LGTM!The helper methods correctly updated to return
FacingClassinstead ofDirStruct, maintaining consistency with the virtual method signature changes. The implementation pattern remains sound.
373-377: LGTM!Both new methods are well-defined:
Electric_Zapcorrectly uses const reference forCoordStructDrawALinkToappropriately passes small POD structs by valueThe method signatures are consistent with existing patterns in the codebase.
455-456: LGTM!The new methods
GetElevationBonusDistanceandTryNextPlanningTokenNodeare straightforward additions with clear signatures. They integrate well with the existing codebase structure.Also applies to: 495-496
638-639: LGTM! Good naming improvement.Renaming
DiskLaserTimertoROFTimer(Rate Of Fire) is a positive change that improves code clarity and makes the purpose more general. The separation between the timer (ROFTimer) and the rate value (ROF) is logical and clear.
712-712: LGTM! Good naming improvement.Renaming
unknown_bool_432toWasSelectedclarifies the purpose of this boolean member, improving code readability and maintainability.
194-194: Virtual method signatures have been properly updated; no derived class overrides exist in this codebase.The virtual method signatures for
TurretFacingandGetRealFacingin TechnoClass have been correctly changed to useFacingClass*parameters, and the corresponding helper methods at lines 509–520 have been appropriately updated to match. No derived classes within this wrapper library override these methods, so no additional updates are required.
370-371: Parameter type change requires verification of external callers and game code function signature.The first parameter type changed from
ObjectClass*toAbstractClass*. While this type widening is valid in the inheritance hierarchy (ObjectClass derives from AbstractClass), verification is limited because:
- No internal callers exist in this repository
- The actual function signature at address 0x6FD210 (external game code) cannot be verified
- External code consuming this API would be affected by this change
Ensure that the game function at 0x6FD210 actually accepts
AbstractClass*and review any external call sites that might be affected.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Helpers/Cast.h (1)
89-91: Typo: "suppored" should be "supported".✏️ Proposed fix
static_assert(!std::is_base_of<AbstractTypeClass, Base>::value || !std::is_abstract<Base>::value, - "abstract_cast: Abstract types (not fully implemented classes) derived from AbstractTypeClass are not suppored."); + "abstract_cast: Abstract types (not fully implemented classes) derived from AbstractTypeClass are not supported.");
🧹 Nitpick comments (3)
Helpers/Cast.h (3)
36-42: Document the precondition forSkipNullCheck=true.When
SkipNullCheckistrue, the null check is bypassed butpAbstract->WhatAmI()is still called unconditionally on line 42. This is a valid optimization pattern, but callers must guaranteepAbstract != nullptrwhen usingSkipNullCheck=true, otherwise undefined behavior (null pointer dereference) occurs.Consider adding a comment or
assertto document this precondition:📝 Suggested documentation
if constexpr (!SkipNullCheck) { if (!pAbstract) return nullptr; } + else + { + // Caller guarantees pAbstract != nullptr when SkipNullCheck is true + } return pAbstract->WhatAmI() == Base::AbsID ? static_cast<T>(pAbstract) : nullptr;
63-69: Same null-dereference risk asspecific_cast.Line 69 accesses
pAbstract->AbstractFlagsunconditionally after the conditional null check. WhenSkipNullCheck=true, callers must ensurepAbstractis non-null.
96-119: Consider adding a brief comment explaining the macro strategy.The macros effectively reduce boilerplate for generating 4 specializations per type (const/non-const × SkipNullCheck true/false). However, macro-heavy code can be difficult to debug and navigate. A brief comment explaining the pattern would help maintainability.
📝 Suggested documentation
+// Macros to generate abstract_cast specializations for each type. +// Each APPLY_* macro generates 4 specializations: +// - non-const, SkipNullCheck=true +// - non-const, SkipNullCheck=false +// - const, SkipNullCheck=true +// - const, SkipNullCheck=false +// APPLY_ABSTRACT_CAST: returns pAbstract directly (identity cast) +// APPLY_GC_ABSTRACT_CAST: uses generic_cast for ObjectClass hierarchy +// APPLY_RT_ABSTRACT_CAST: uses reinterpret_cast via TechnoClass for MissionClass/RadioClass `#define` ABSTRACT_PARAM pAbstract
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Helpers/Cast.h
🔇 Additional comments (4)
Helpers/Cast.h (4)
16-21: LGTM!The non-const overload correctly forwards to the const version, preserving the
SkipNullCheckparameter through the call chain.
99-100:reinterpret_castbypasses type safety forMissionClass*andRadioClass*.
RT_GENERIC_CASTfirst validates viageneric_cast<TechnoClass*>, then usesreinterpret_castto convert to the target type. This assumesMissionClassandRadioClassare bases ofTechnoClasswith compatible memory layouts. While this may be correct for this codebase's class hierarchy,reinterpret_castprovides no compile-time or runtime validation.If the inheritance hierarchy is
MissionClass -> RadioClass -> TechnoClass, consider usingstatic_castinstead, which would provide compile-time verification:-#define RT_GENERIC_CAST(TypePointer, ConstQual, SkipNullCheck) \ - reinterpret_cast<ConstQual TypePointer>(GENERIC_CAST(TechnoClass*, ConstQual, SkipNullCheck)) +#define RT_GENERIC_CAST(TypePointer, ConstQual, SkipNullCheck) \ + static_cast<ConstQual TypePointer>(GENERIC_CAST(TechnoClass*, ConstQual, SkipNullCheck))Please verify whether
static_castworks with your class hierarchy, or document whyreinterpret_castis necessary.
121-126: LGTM on the macro applications.The specialization coverage is complete:
AbstractClass*- identity castObjectClass*,TechnoClass*,FootClass*- usegeneric_cast(flag-based validation)MissionClass*,RadioClass*- use reinterpret via TechnoClass
128-134: Good practice: cleaning up macros after use.Undefining the macros prevents pollution of the global namespace and potential conflicts in files that include this header.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
Extension some function.
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.