Skip to content

chore: Log bad weapon radii - #3359

Open
Stubbjax wants to merge 1 commit into
TheSuperHackers:mainfrom
Stubbjax:log-bad-weapon-radii
Open

Stubbjax wants to merge 1 commit into
TheSuperHackers:mainfrom
Stubbjax:log-bad-weapon-radii

Conversation

@Stubbjax

Copy link
Copy Markdown

This change adds logging for detecting erroneous weapon radii. This applies to cases where a weapon template's PrimaryDamageRadius is smaller than its projectile object's GeometryMajorRadius, which can result in inconsistent damage being applied to targets.

See TheSuperHackers/GeneralsGamePatch2#193 for more info.

@Stubbjax Stubbjax self-assigned this Sep 25, 2026
@Stubbjax Stubbjax added Gen Relates to Generals ZH Relates to Zero Hour Debug Is mostly debug functionality labels Sep 25, 2026
@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a2ad307a-d3b0-4545-a85c-7c4b578e35c9

📥 Commits

Reviewing files that changed from the base of the PR and between 722dbea and 3916de5.

📒 Files selected for processing (2)
  • Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
  • GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

Both game engine variants add a debug diagnostic during weapon template post-processing. It logs when a projectile’s major geometry radius exceeds the primary damage radius of a weapon with positive primary damage.

Changes

Projectile radius diagnostic

Layer / File(s) Summary
Add the debug radius check
Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp, GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp
In both variants, WeaponTemplate::postProcessLoad logs the weapon name and radius values when a resolved projectile has positive primary damage and its major geometry radius exceeds PrimaryDamageRadius.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Other

Suggested reviewers: xezon

Merge Risk: ⚪ Minimal · up to 3916d

This change adds debug diagnostics in both game variants, with no supported gameplay regression. No actionable merge-blocking risk remains.

Architecture Summary

Architecture risk: 🔵 Low · up to 3916d

The change affects 2 systems.

Changed systems: Generals, GeneralsMD

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — Generals (service) was modified; 1 changed file maps to changed impact.
  • observed — GeneralsMD (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in Generals/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp: Added a debug-only diagnostic for projectile weapons with positive primary damage whose primary damage radius is less than the projectile template’s major geometry radius; it logs the weapon and both radius values.
  • observed — Modified behavior in GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp: WeaponTemplate::postProcessLoad adds an RTS_DEBUG check for a resolved projectile when primary damage is positive; it logs when the configured primary damage radius is smaller than the projectile’s major geometry radius.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding logs for invalid weapon radius relationships.
Description check ✅ Passed The description directly explains the added logging, the affected code identifiers, and the radius mismatch that the change detects.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Low risk] Adds debug logging to weapon configuration validation.

The PR appears safe to merge, but the diagnostic will miss bad radii introduced by map-specific weapon overrides.

Summary

Adds a debug-build warning in both game variants when a weapon's primary damage radius is smaller than its projectile template's major geometry radius.

  • The check runs during weapon-template post-processing; map-specific overrides loaded later are not covered.

Reviews (1) · Last reviewed commit: "chore: Log bad weapon radii"

m_projectileTmpl = TheThingFactory->findTemplate(m_projectileName);
DEBUG_ASSERTCRASH(m_projectileTmpl, ("projectile %s not found!",m_projectileName.str()));

#if RTS_DEBUG

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postProcessLoad (xfer saveload) is not the right place for it. It needs to go into a validate function after INI parse (WeaponStore::parseWeaponTemplateDefinition).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's where I originally put it, but m_projectileTmpl is null here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is unfortunate. There currently is no standardized way of calling validate after INI parsing, but in this case it would need to be called at the end of all INI parse then. Maybe we should add a interface for INI classes, similar to SubsystemInterface.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's effectively what postProcessLoad does though? It runs after all INI data has been parsed so that any references/data can be assigned/validated.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh you are right. I was not aware. Maybe we need to consolidate the validate functions and postProcessLoad. I will take a look.

if (m_projectileTmpl && m_primaryDamage > 0.0)
{
const Real projectileRadius = m_projectileTmpl->getTemplateGeometryInfo().getMajorRadius();
if (m_primaryDamageRadius < projectileRadius)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While at it, maybe also add a log for when secondary radius is smaller equal primary radius.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Debug Is mostly debug functionality Gen Relates to Generals ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants