Skip to content

feat(client): Add SelectionCircle option drawing a ring under selected objects - #3220

Open
triatomic wants to merge 2 commits into
TheSuperHackers:mainfrom
triatomic:qol/selection-circle
Open

feat(client): Add SelectionCircle option drawing a ring under selected objects#3220
triatomic wants to merge 2 commits into
TheSuperHackers:mainfrom
triatomic:qol/selection-circle

Conversation

@triatomic

Copy link
Copy Markdown

Adds an opt-in client option drawing a green ring on the ground under every selected object, so the current selection reads at a glance. Defaults off; retail behavior unless set in Options.ini.

SelectionCircle (= Yes) — the ring is a projected decal through the shadow system rather than screen-space lines, so it wraps the terrain and the model genuinely occludes it. It lives in its own decal slot (selecting a horde unit does not evict its horde ring) and is released alongside the shadow and terrain decal when the render object is torn down — left behind it would dangle in the projected shadow manager and crash the next renderShadows. Sized from the bounding circle, scaled per kind: a building fills its circle, a soldier barely occupies the middle of one.

Asset dependency, up front: the decal expects a PlainRingSelection.tga in the game's assets — the engine appends the extension and tints the art green at runtime, so a plain white or greyscale ring works. This repo ships no such texture; the Contra mod provides its own. With the option on but no texture present, no ring renders. Happy to contribute the texture through whatever channel fits your asset pipeline, or the name can be pointed at an existing decal.

The drawing lives in the shared W3D model draw code with a default no-op in both titles' draw-module interface; the option currently drives it from Zero Hour's Drawable only. A Generals replica can follow after review.

This option ships in the Contra mod's engine fork and has been played there; this PR is the port onto current main, collapsing the fork's five ring commits into their final state with the teardown fix folded in. Code was written with LLM assistance and human-reviewed, adapted and playtested by the author.

Prepared for Squash and Merge.

…d objects

Options.ini: SelectionCircle = Yes draws a green ring on the ground under
every selected object, so the current selection reads at a glance.

The ring is a projected decal through the shadow system rather than
screen space lines, so it wraps the terrain and the model genuinely
occludes it. It lives in its own decal slot, so selecting a horde unit
does not evict its horde ring, and it is released alongside the shadow
and terrain decal when the render object is torn down - left behind it
would dangle in the projected shadow manager and crash the next
renderShadows.

Sized from the bounding circle, scaled per kind - a building fills its
circle, a soldier barely occupies the middle of one. Expects a
PlainRingSelection.tga in the mod's assets; the art is tinted green at
runtime, so a plain white ring works.

The drawing lives in the shared W3D model draw code with a default no-op
in both titles' draw module interface; the option currently drives it
from Zero Hour's Drawable only. A Generals replica can follow after
review.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Add opt-in projected selection circles for Zero Hour

✨ Enhancement 🐞 Bug fix 🕐 20-40 Minutes

Grey Divider

AI Description

• Adds opt-in terrain-projected green rings for selected Zero Hour objects.
• Sizes rings by object geometry using a dedicated decal slot.
• Releases selection decals during deselection and render teardown to prevent dangling references.
Diagram

sequenceDiagram
    participant INI as Options.ini
    participant Prefs as Option Preferences
    participant Global as Global Data
    participant Drawable as Drawable
    participant W3D as W3D Model Draw
    participant Shadows as Shadow Manager
    INI->>Prefs: Read SelectionCircle
    Prefs->>Global: Store enabled flag
    Drawable->>Global: Check option
    Drawable->>Drawable: Scale object radius
    Drawable->>W3D: Set selection decal
    alt selected and eligible
        W3D->>Shadows: Add projected ring
    else deselected or teardown
        W3D->>Shadows: Release ring
    end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Screen-space selection overlay
  • ➕ Requires no projected-shadow resource ownership
  • ➕ Can avoid a world-space texture dependency
  • ➖ Does not conform to terrain
  • ➖ Cannot be naturally occluded by models
  • ➖ Looks less integrated with the world
2. Reuse the terrain decal slot
  • ➕ Avoids adding another Shadow pointer and interface method
  • ➕ Uses the existing decal lifecycle
  • ➖ Selection would replace horde or chem-suit decals
  • ➖ Couples unrelated visual states
3. Reuse or bundle an existing ring asset
  • ➕ Makes the option render without mod-provided assets
  • ➕ Avoids silent absence when PlainRingSelection.tga is missing
  • ➖ Requires asset-pipeline agreement
  • ➖ An existing texture may not support neutral runtime tinting

Recommendation: Keep the dedicated projected-decal approach because it provides terrain conformity and model occlusion without displacing gameplay decals. Screen-space rendering and slot reuse compromise the intended visuals or existing effects; separately confirming an approved PlainRingSelection asset is the main integration follow-up.

Files changed (10) +137 / -1

Enhancement (7) +119 / -1
OptionPreferences.hExpose the SelectionCircle preference accessor +1/-0

Expose the SelectionCircle preference accessor

• Declares a const accessor for reading the opt-in SelectionCircle value from user preferences.

Core/GameEngine/Include/Common/OptionPreferences.h

W3DModelDraw.hAdd dedicated W3D selection decal ownership +5/-0

Add dedicated W3D selection decal ownership

• Adds the draw-module override and a separate Shadow pointer so selection rings do not share the terrain decal slot.

Core/GameEngineDevice/Include/W3DDevice/GameClient/Module/W3DModelDraw.h

W3DModelDraw.cppCreate and safely release projected selection rings +60/-0

Create and safely release projected selection rings

• Creates a green, world-aligned alpha decal using 'PlainRingSelection' and the supplied radius. Initializes and releases the dedicated resource during decal changes and render-object teardown to avoid dangling projected-shadow references.

Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp

DrawModule.hExtend the Generals draw-module interface +3/-0

Extend the Generals draw-module interface

• Adds a default no-op selection decal hook, preserving compatibility while allowing shared W3D code to implement the feature.

Generals/Code/GameEngine/Include/Common/DrawModule.h

DrawModule.hExtend the Zero Hour draw-module interface +3/-0

Extend the Zero Hour draw-module interface

• Adds a default no-op selection decal hook used by Zero Hour Drawable and implemented by W3DModelDraw.

GeneralsMD/Code/GameEngine/Include/Common/DrawModule.h

Drawable.hDeclare selection decal synchronization +3/-0

Declare selection decal synchronization

• Declares the client-only Drawable helper that synchronizes selection state with the render decal.

GeneralsMD/Code/GameEngine/Include/GameClient/Drawable.h

Drawable.cppDrive rings from Zero Hour selection events +44/-1

Drive rings from Zero Hour selection events

• Shows or hides a ring on the first draw module when eligible objects are selected or deselected. Computes radius from bounding geometry with structure, infantry, and general scaling factors plus a minimum size.

GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp

Other (3) +18 / -0
OptionPreferences.cppParse SelectionCircle from Options.ini +14/-0

Parse SelectionCircle from Options.ini

• Recognizes 'SelectionCircle = Yes' case-insensitively and defaults the feature to disabled when absent.

Core/GameEngine/Source/Common/OptionPreferences.cpp

GlobalData.hStore the client selection-circle setting +2/-0

Store the client selection-circle setting

• Adds a client-side global flag controlling whether selected objects receive projected rings.

GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h

GlobalData.cppLoad SelectionCircle into Zero Hour global data +2/-0

Load SelectionCircle into Zero Hour global data

• Defaults selection circles off and overrides the flag from OptionPreferences during global data parsing.

GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp

@greptile-apps

greptile-apps Bot commented Aug 26, 2026

Copy link
Copy Markdown

Greptile Summary

The PR adds an opt-in, projected selection-ring decal for Zero Hour and preserves its requested state across render-object lifecycle changes.

  • Reads the SelectionCircle option into client global data.
  • Adds selection-decal ownership, visibility handling, resizing, and teardown to W3DModelDraw.
  • Recreates a selected object's decal after model replacement, but does not preserve hidden visibility for that recreated decal.

Confidence Score: 4/5

The PR is not yet safe to merge because model replacement can expose the selection ring of an effectively hidden drawable.

The replacement path recreates the selection decal with rendering forced on, while its subsequent hidden-state restoration disables only the render object and normal shadow.

Files Needing Attention: Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp

Important Files Changed

Filename Overview
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp Adds selection-decal creation and lifecycle handling, but model replacement can re-enable the decal for an effectively hidden drawable.
Core/GameEngineDevice/Include/W3DDevice/GameClient/Module/W3DModelDraw.h Adds independent selection-decal ownership and remembered requested state used during render-object replacement.
GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp Connects selection and geometry changes to decal updates and derives decal size from drawable geometry.
Core/GameEngine/Source/Common/OptionPreferences.cpp Adds opt-in parsing for SelectionCircle with disabled behavior when the option is absent.
GeneralsMD/Code/GameEngine/Source/Common/GlobalData.cpp Initializes the feature off and imports the client preference during global-data parsing.
Prompt To Fix All With AI
### Issue 1
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp:3147-3150
**Hidden ring becomes visible**

When a selected, effectively hidden drawable replaces its model, this block recreates the selection decal with rendering enabled, while the subsequent hidden-state restoration disables only the render object and normal shadow. The green ring therefore renders beneath the hidden drawable and reveals its location.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (2): Last reviewed commit: "fix(client): Keep the selection ring thr..." | Re-trigger Greptile

Comment thread Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp Outdated
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 26, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Terrain updates remove selection ring ✓ Resolved 🐞 Bug ≡ Correctness
Description
setTerrainDecal() releases the supposedly independent m_selectionDecal, so any horde, chem-suit,
fake-object, or model-condition terrain-decal update permanently removes the ring from an object
that remains selected. No subsequent selection callback recreates it.
Code

Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[R2773-2777]

+	// TheSuperHackers @feature drop the selection ring too
+	if (m_selectionDecal)
+	{
+		m_selectionDecal->release();
+		m_selectionDecal = nullptr;
Evidence
The new method documents that the slots are independent, but the changed terrain-decal path
explicitly releases the selection slot. Runtime horde changes call this path, and repository search
shows the only selection-decal refresh calls are onSelected() and onUnselected().

Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[2727-2732]
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[2768-2784]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/HordeUpdate.cpp[326-358]
GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp[1022-1048]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Terrain decal changes currently release the active selection decal, although the feature requires independent slots.
## Issue Context
`Drawable::setTerrainDecal()` is used at runtime for horde and other status decals, while selection decal refresh only occurs on selection transitions.
## Fix Focus Areas
- Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[2768-2780]
- GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp[988-1020]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Model swaps lose selection ring ✓ Resolved 🐞 Bug ≡ Correctness
Description
nukeCurrentRender() releases the selection decal during a selected object's model swap, but the
selection request is not stored and the replacement render-object path never recreates it. Any
condition-state transition that changes the model therefore leaves the selected object without a
ring.
Code

Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[R2849-2851]

+	if (m_selectionDecal)
+		m_selectionDecal->release();
+	m_selectionDecal = nullptr;
Evidence
The only retained selection state in W3DModelDraw is the decal pointer itself. The model-change
branch destroys that pointer, creates a replacement render object, and performs post-creation setup
without calling setSelectionDecal() or otherwise restoring the ring.

Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[2736-2765]
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[2831-2862]
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[3064-3089]
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[3091-3105]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A model-state render-object replacement correctly releases the old bound decal but never binds a new selection decal.
## Issue Context
Preserve the requested selection state/radius independently from the current `Shadow*`, then recreate the decal after a new render object is installed.
## Fix Focus Areas
- Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[2736-2765]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[2831-2851]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[3076-3105]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Ring ignores visibility transitions ✓ Resolved 🐞 Bug ≡ Correctness
Description
The decal samples shroud state only when created and is unconditionally render-enabled, while
setHidden() and setFullyObscuredByShroud() update only the existing shadow and terrain decal. A
selected drawable can therefore keep rendering its ring after it becomes hidden or fully shrouded,
potentially exposing a unit that the client should no longer display.
Code

Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[R2760-2761]

+		m_selectionDecal->enableShadowInvisible(m_fullyObscuredByShroud);
+		m_selectionDecal->enableShadowRender(TRUE);
Evidence
Creation sets the shroud flag once and then forces rendering on. The module's later hidden and
shroud callbacks explicitly synchronize m_shadow and m_terrainDecal but omit the newly added
m_selectionDecal.

Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[1828-1840]
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[1944-1955]
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[2757-2764]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Selection decals do not follow hidden and shroud transitions after creation.
## Issue Context
Apply the same hidden/render and shroud-invisibility updates used for `m_terrainDecal` to `m_selectionDecal`, and honor current visibility when initially creating it.
## Fix Focus Areas
- Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[1828-1840]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[1944-1955]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp[2757-2764]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Geometry changes leave stale radius ✓ Resolved 🐞 Bug ≡ Correctness
Description
The ring radius is calculated only during selection transitions, while
Drawable::reactToGeometryChange() never refreshes the decal. A selected object whose runtime
geometry changes keeps a ring sized for its old bounding circle.
Code

GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp[R1010-1012]

+		radius = getDrawableGeometryInfo().getBoundingCircleRadius() * scale;
+		if( radius < 1.0f )
+			radius = 1.0f;
Evidence
The new code derives radius directly from current geometry. The geometry-change callback only
forwards to draw modules, and the only calls to updateSelectionDecal() are the selection and
unselection callbacks, so no resize occurs after a geometry update.

GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp[988-1018]
GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp[1022-1048]
GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp[4081-4085]
GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp[4205-4210]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Selection-circle size becomes stale when an object's geometry changes while it remains selected.
## Issue Context
The radius derives from the current drawable bounding circle, but recalculation currently occurs only in selection callbacks.
## Fix Focus Areas
- GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp[988-1020]
- GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp[4205-4210]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can start a comment with 'qodo' or '@qodo' to chat about any finding

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/Drawable.cpp
…geometry changes

Addresses the review findings:

- setTerrainDecal no longer drops the ring: a horde or chem suit decal
  update while the unit is selected left it ringless.

- The wanted state and radius are remembered in the draw module, and the
  ring is recreated after a render object rebuild - a damage state or
  upgrade swaps the model while the object stays selected, and the ring
  used to vanish until reselection.

- Hidden and fully shrouded transitions now disable the ring's render
  like the shadow and terrain decal, so it cannot expose a unit the
  client should not display.

- reactToGeometryChange resizes the ring along with the geometry its
  radius is derived from.
Comment on lines +3147 to +3150
// TheSuperHackers @fix The selection ring was bound to the render object that was just
// torn down; the object is still selected, so put the ring back on the new one.
if (m_selectionDecalWanted)
setSelectionDecal(TRUE, m_selectionDecalRadius);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Hidden ring becomes visible

When a selected, effectively hidden drawable replaces its model, this block recreates the selection decal with rendering enabled, while the subsequent hidden-state restoration disables only the render object and normal shadow. The green ring therefore renders beneath the hidden drawable and reveals its location.

Knowledge Base Used: Game client runtime

Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp
Line: 3147-3150

Comment:
**Hidden ring becomes visible**

When a selected, effectively hidden drawable replaces its model, this block recreates the selection decal with rendering enabled, while the subsequent hidden-state restoration disables only the render object and normal shadow. The green ring therefore renders beneath the hidden drawable and reveals its location.

**Knowledge Base Used:** [Game client runtime](https://app.greptile.com/thesuperhackers/-/custom-context/knowledge-base/thesuperhackers/generalsgamecode/-/docs/game-client-runtime.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant