Repository navigation
Conversation
Caball009
added
Enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe changes update tint-envelope access and transfer for Overlord riders. They also add non-retail-compatible handling for powered objects: brownout signaling for qualifying zero-energy objects, disabled tint during initialization, and attack rejection while underpowered. ChangesTint-envelope transfer
Powered-object behavior
Priority: Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested labels: Suggested reviewers: Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue [#7] requires no crash, no attack or turret turning, a black texture, a lightning symbol, and free transporter movement when the portable Avenger turret loses power. The null-safe `Drawable::set… |
Implement the complete underpowered behavior for issue [#7]. Add the lightning-symbol indicator and block turret turning while DISABLED_UNDERPOWERED is active. Apply the attack restriction in every intended build configuration. Add regres… |
-
Check name Status Explanation Title check The title clearly summarizes the main change: support for objects with the POWERED KindOf type when they have no power. Out of Scope Changes check The Drawablechanges prevent null tint-envelope dereferences in rider draw paths. TheObject,Energy, and tint changes implement underpowered powered-object behavior. These changes directly supp…Description check The pull request description directly matches the changeset. It describes the crash fix, underpowered tint behavior, and no-retail energy logic implemented in the modified files. Full details: Out of Scope Changes check
-
Explanation
The
Drawablechanges prevent null tint-envelope dereferences in rider draw paths. TheObject,Energy, and tint changes implement underpowered powered-object behavior. These changes directly support issue [#7].
-
Full details: Linked Issues check
-
Explanation
Issue [
#7] requires no crash, no attack or turret turning, a black texture, a lightning symbol, and free transporter movement when the portable Avenger turret loses power. The null-safeDrawable::setColorTintEnvelopecall addresses the reported crash.Object::initObjectappliesTINT_STATUS_DISABLEDto newly initialized underpowered objects, andObject::isAbleToAttackblocks attacks only whenRETAIL_COMPATIBLE_CRCis disabled. The current description confirms that the lightning symbol is still absent. The reviewed changes do not establish that the turret cannot turn or that the transporter remains movable. No regression test evidence is present.Resolution
Implement the complete underpowered behavior for issue [
#7]. Add the lightning-symbol indicator and block turret turning whileDISABLED_UNDERPOWEREDis active. Apply the attack restriction in every intended build configuration. Add regression coverage for the crash and the powered Avenger behavior, including transporter movement.
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
19178bd7-d94a-4f90-903d-ecfebd6d3e0f
📒 Files selected for processing: 7
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DOverlordAircraftDraw.cppCore/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DOverlordTankDraw.cppCore/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DOverlordTruckDraw.cppGenerals/Code/GameEngine/Include/GameClient/Drawable.hGenerals/Code/GameEngine/Source/GameLogic/Object/Object.cppGeneralsMD/Code/GameEngine/Include/GameClient/Drawable.hGeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Underpowered turrets can still rotate through recentering and idle… · Object.cpp:2995-2999
Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp:2995-2999
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnderpowered turrets can still rotate through recentering and idle scanning.
The
DISABLED_UNDERPOWEREDcheck inObject::isAbleToAttack()blocks only the object-target attack path. The turret recentering and idle-scan states callfriend_turnTowardsAngle()with no power check. An underpowered turret can therefore keep turning, which Issue#7does not allow.Apply the power check in both
Generals/andGeneralsMD/, because each variant has its ownTurretAI.cpp. A check in the shared turn helper would cover the recentering, idle-scan, and object-aim states in each variant. The body of that helper was not inspected, so no diff is given.
🟡 Minor · Allocate the rider's tint envelope before copying the carrier tint. · Drawable.h:317-318
Generals/Code/GameEngine/Include/GameClient/Drawable.h:317-318
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAllocate the rider's tint envelope before copying the carrier tint.
When a rider is added after the carrier becomes underpowered, the containment path does not propagate the carrier's existing disabled state. The new rider
Drawablestarts with a null tint envelope. Each Overlord draw path then callssetColorTintEnvelope(...), but the setter skips the copy. The rider can therefore miss the dark underpowered tint.Update the shared setter in both game-variant headers.
Suggested fix
--- a/Generals/Code/GameEngine/Include/GameClient/Drawable.h +++ b/Generals/Code/GameEngine/Include/GameClient/Drawable.h @@ - void setColorTintEnvelope( const TintEnvelope* source ) { if (m_colorTintEnvelope && source) *m_colorTintEnvelope = *source; } + void setColorTintEnvelope( const TintEnvelope* source ) + { + if (source) + { + if (m_colorTintEnvelope == nullptr) + m_colorTintEnvelope = newInstance(TintEnvelope); + *m_colorTintEnvelope = *source; + } + } --- a/GeneralsMD/Code/GameEngine/Include/GameClient/Drawable.h +++ b/GeneralsMD/Code/GameEngine/Include/GameClient/Drawable.h @@ - void setColorTintEnvelope( const TintEnvelope* source ) { if (m_colorTintEnvelope && source) *m_colorTintEnvelope = *source; } + void setColorTintEnvelope( const TintEnvelope* source ) + { + if (source) + { + if (m_colorTintEnvelope == nullptr) + m_colorTintEnvelope = newInstance(TintEnvelope); + *m_colorTintEnvelope = *source; + } + }
🧹 Nitpick comments: 2
Generals/Code/GameEngine/Source/Common/RTS/Energy.cpp: 1
128-129: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLimit zero-energy power updates to the affected object.
A zero-energy
KINDOF_POWEREDobject reachesaddConsumption(0)on both entry and exit. This call still scans all player objects throughPlayer::onPowerBrownOutChange, although the energy state does not change. Apply the current brownout state directly to the entering object. Remove the player-wide refresh from the zero-energy exit path.Suggested fix
#if !RETAIL_COMPATIBLE_CRC - else if (obj->isKindOf(KINDOF_POWERED)) - addConsumption(-energy); + else if (obj->isKindOf(KINDOF_POWERED)) { + if (hasSufficientPower()) + obj->clearDisabled(DISABLED_UNDERPOWERED); + else + obj->setDisabled(DISABLED_UNDERPOWERED); + } #endif @@ -#if !RETAIL_COMPATIBLE_CRC - else if (obj->isKindOf(KINDOF_POWERED)) - addConsumption(energy); -#endifGeneralsMD/Code/GameEngine/Source/Common/RTS/Energy.cpp: 1
147-148: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the player-wide refresh for zero-energy powered objects.
When
energy == 0and the object isKINDOF_POWERED,addConsumption(0)still callsPlayer::onPowerBrownOutChange, which iterates the player's objects. The same unnecessary scan occurs when the object leaves.Synchronize only the entering object with
hasSufficientPower(). Remove the zero-energy exit callback. Apply the same change in Generals and GeneralsMD.Suggested fix
#if !RETAIL_COMPATIBLE_CRC - else if (obj->isKindOf(KINDOF_POWERED)) - addConsumption(-energy); + else if (obj->isKindOf(KINDOF_POWERED)) { + if (hasSufficientPower()) + obj->clearDisabled(DISABLED_UNDERPOWERED); + else + obj->setDisabled(DISABLED_UNDERPOWERED); + } #endif-#if !RETAIL_COMPATIBLE_CRC - else if (obj->isKindOf(KINDOF_POWERED)) - addConsumption(energy); -#endif
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
fb6eac9b-42e5-433f-891e-4a3f0baa40be
📒 Files selected for processing: 2
Generals/Code/GameEngine/Source/Common/RTS/Energy.cppGeneralsMD/Code/GameEngine/Source/Common/RTS/Energy.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
311a978 to
be68507
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
-
⚙️ Run configuration
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
6db375ef-f6e8-4cf0-a696-c4e0e3f4452f
📒 Files selected for processing: 2
-
GeneralsMD/Code/GameEngine/Source/Common/RTS/Energy.cppGeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
-
8b39d33 to
cffe846
Compare
Caball009
added
the
NoRetail
… built without sufficient power.
fb56f09 to
08d6abf
Compare
 [Medium impact] No new blocking issue was established in the changes since the previous review.
|
| // TheSuperHackers @info clearDisabled is only called here for correctness, ensuring the data | ||
| // is cleared if the object is about to be destroyed, since Object's destructor doesn't clear this. | ||
| else if (obj->isKindOf(KINDOF_POWERED)) | ||
| obj->clearDisabled(DISABLED_UNDERPOWERED); |
There was a problem hiding this comment.
Moving a unit to another team while underpowered goes through this, so it gets re-enabled and then disabled again right after when it joins the new team. Maybe only do this on destroy?
| // TheSuperHackers A unit module that requires to be powered cannot attack without power. | ||
| if (isDisabledByType(DISABLED_UNDERPOWERED)) |
There was a problem hiding this comment.
Comment missing tag/date.
Affects every underpowered object, not just units. AI auto-acquire skips objects that can’t attack, so enemies would stop attacking underpowered units and structures.
Attack orders and guard logic change too I think.
There was a problem hiding this comment.
I've removed this as it doesn't seem necessary, anyway.
08d6abf to
773acdb
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
773acdb to
2adabd0
Compare
| // explicitly initialize first field as nullptr as Object::pauseAllSpecialPowers relies on it, | ||
| // which may be called downstream of Player::onPowerBrownOutChange and Object::setTeam | ||
| m_behaviors[0] = nullptr; |
There was a problem hiding this comment.
Strictly unnecessary but would otherwise implicitly rely on the memory manager which zeroes out newly allocated memory. A lot of code relies on this behavior, though.
This PR makes 3 changes:
nullptrdereference inDrawable::setColorTintEnvelope(see issue). The USA Avenger laser turret now turns dark when underpowered (if set toPOWERED), but no low power icon because the unit itself is still responsive.See commits for clean diffs.
TODO: