refactor(ww3d2): Replace Vector3 reference casts with explicit field copies by CryoTheRenegade · Pull Request #3580 · TheSuperHackers/GeneralsGameCode

CryoTheRenegade · 2026-10-23T21:37:42Z

Some WW3D2 code reads or writes a Vector4 or TPoint3D<float> through a Vector3 reference. Accessing an object through a reference to an unrelated type is undefined behavior, the same problem #3521 tracks for float/integer casts. #3532 fixed those casts with std::bit_cast, but bit_cast can't fix these sites. Vector3 and Vector4 have user-provided copy constructors that aren't trivially copyable, and a Vector4 is larger than a Vector3. This PR copies the fields instead.

Changes

Init_From_INI in both games read 5 TPoint3D<float> values through Vector3 references. It now builds Vector3(tp.X,tp.Y,tp.Z).
Render_Dazzle in both games wrote a whole Vector3 over vertex->x at 8 places. It now stores x, y and z separately.
Recolor(unsigned&) in colorspace.h recolored the RGB part of a Vector4 through a Vector3 reference. It now recolors a Vector3 copy and converts it with the original alpha.
W3DAssetManager::Recolor_Vertices in both games had the same cast. It now recolors a copy and writes the RGB back with rgba.Set, which keeps alpha, before the existing clamp.

DX8Wrapper::Convert_Color(const Vector4&) and Convert_Color_Clamp have the same cast and stay as they are for now. On VC6, Convert_Color(const Vector3&, float) is inline assembly that reads through the reference. A temporary Vector3 there adds a stack copy of three floats to every call, including per-vertex loops, so that change needs its own PR.

The DazzleINIClass downcast in Init_From_INI also stays. ww3d.cpp passes a plain INIClass, so changing the cast wouldn't fix it.

Testing

Generals and Zero Hour, with tools, build with VS 2026 and VC6 in Release and Debug.

I compared the disassembly of every rebuilt object against a build of main.

With VS 2026 Release, three functions differ per game. Init_From_INI loads the three floats before storing them. Recolor(unsigned&) converts alpha after the RGB call instead of before it. Copy_Image_Generate_Mipmap inlines Recolor. The arithmetic and the stored values are the same. Render_Dazzle and Recolor_Vertices compile to the same code.
With VC6 Release, the same functions differ. VC6 also stops inlining Recolor and Convert_Color(unsigned) into the texture copy functions in bitmaphandler.cpp and ddsfile.cpp. In the Generals Render_Dazzle, two Matrix4x4 temporaries now share a stack slot, and the vertex stores keep the same structure. VC6 copies the new Vector3 temporaries in Init_From_INI through the x87 stack. That would quiet a signaling NaN, but these values come from INI text, which can't produce one.

AI Disclosure

Claude Opus 5.5 was used to aid in implementation and to compare the disassembly.

…copies Dazzle read TPoint3D<float> values through Vector3 references and wrote whole Vector3 values over the x member of its vertices. Recolor(unsigned&) and W3DAssetManager::Recolor_Vertices recolored the RGB part of a Vector4 through a Vector3 reference. These accesses go through an unrelated type. Build Vector3 values from the point members, store the vertex x, y and z fields one by one, and recolor an RGB copy whose result is written back with the original alpha. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

chatgpt-codex-connector · 2026-10-23T21:37:54Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-23T21:38:45.977862Z We couldn’t complete this review. Comment `@codex review` to try again.	`e7999a9`	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

coderabbitai · 2026-10-23T21:38:02Z

No actionable comments were generated in the recent review.

ℹ️ Recent 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: 203696b8-90cf-4898-8dd8-bc488310d62a

📥 Commits

Reviewing files that changed from the base of the PR and between f02fcd3 and e7999a9.

📒 Files selected for processing: 5

Core/Libraries/Source/WWVegas/WW3D2/colorspace.h
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DAssetManager.cpp
Generals/Code/Libraries/Source/WWVegas/WW3D2/dazzle.cpp
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DAssetManager.cpp
GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/dazzle.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.

Walkthrough

The changes replace vector reinterpret casts in color and dazzle code. Recoloring now handles RGB separately while preserving alpha. Dazzle configuration and quad positions use explicit component assignments.

Changes

RGB and Dazzle Vector Handling

Layer / File(s)	Summary
Separate RGB recoloring from alpha `Core/Libraries/Source/WWVegas/WW3D2/colorspace.h`, `Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DAssetManager.cpp`, `GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DAssetManager.cpp`	The recolor overload and both asset managers now recolor RGB separately and preserve the original alpha.
Copy dazzle configuration components `Generals/Code/Libraries/Source/WWVegas/WW3D2/dazzle.cpp`, `GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/dazzle.cpp`	Dazzle initialization now copies flare colors, direction, and dazzle and halo colors from individual point components.
Assign dazzle quad positions `Generals/Code/Libraries/Source/WWVegas/WW3D2/dazzle.cpp`, `GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/dazzle.cpp`	Dazzle and halo quad positions now assign X, Y, and Z individually. Vertex ordering and other attributes are unchanged.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: xezon, bobtista

Merge Risk: Minimal · up to e7999

No concrete current-head correctness or availability risk is established; the change is mergeable after normal checks.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: replacing Vector3 reference casts with explicit field copies.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The description explains the undefined-behavior risk and the field-copy changes to recoloring, INI values, and vertex coordinates. It also identifies related casts that remain unchanged and summarizes…

Full details: Description check

Explanation

The description explains the undefined-behavior risk and the field-copy changes to recoloring, INI values, and vertex coordinates. It also identifies related casts that remain unchanged and summarizes the reported testing.

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

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

greptile-apps · 2026-10-23T21:40:12Z

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

Replaces Vector3 reference casts with explicit component copies in both games.

Dazzle setup copies each point's coordinates into its vectors.
Dazzle and halo corners write their coordinates into each vertex.
Color shifts update RGB while keeping alpha attached.

CryoTheRenegade explicitly deferred the casts inside DX8Wrapper::Convert_Color and Convert_Color_Clamp because changing them needs a separate performance review. CryoTheRenegade also intentionally retained the DazzleINIClass cast because changing that cast alone would not fix the caller's use of INIClass.

_{Reviews (1) · Last reviewed commit: "refactor(ww3d2): Replace Vector3 referen..." · Reviewed by Greptile}

xezon

Makes sense. Are there more of these?

CryoTheRenegade · 2026-10-27T19:39:55Z

Makes sense. Are there more of these?

Yes but most of them need special care (See #3605), I will be making follow up prs for those as i get some closed

…copies (TheSuperHackers#3580) Dazzle read TPoint3D<float> values through Vector3 references and wrote whole Vector3 values over the x member of its vertices. Recolor(unsigned&) and W3DAssetManager::Recolor_Vertices recolored the RGB part of a Vector4 through a Vector3 reference. These accesses go through an unrelated type. Build Vector3 values from the point members, store the vertex x, y and z fields one by one, and recolor an RGB copy whose result is written back with the original alpha.

CryoTheRenegade mentioned this pull request Oct 26, 2026

refactor(w3ddevice): Read matrix rows through their components instead of Vector3 casts #3605

Merged

xezon approved these changes Oct 27, 2026

View reviewed changes

xezon added Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing labels Oct 27, 2026

xezon merged commit 1ca1f45 into TheSuperHackers:main Oct 27, 2026
25 checks passed

Navigation Menu

Repository navigation

refactor(ww3d2): Replace Vector3 reference casts with explicit field copies - #3580

refactor(ww3d2): Replace Vector3 reference casts with explicit field copies#3580
xezon merged 1 commit into
TheSuperHackers:mainfrom
CryoTheRenegade:refactor/vector-field-casts

CryoTheRenegade commented Oct 23, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented Oct 23, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 23, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Oct 23, 2026 •

edited

Loading

Summary

Uh oh!

xezon left a comment

Uh oh!

CryoTheRenegade commented Oct 27, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

CryoTheRenegade commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Testing

AI Disclosure

Uh oh!

chatgpt-codex-connector Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

coderabbitai Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

greptile-apps Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Summary

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

CryoTheRenegade commented Oct 27, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CryoTheRenegade commented Oct 23, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Oct 23, 2026 •

edited

Loading

coderabbitai Bot commented Oct 23, 2026 •

edited

Loading

greptile-apps Bot commented Oct 23, 2026 •

edited

Loading