You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(dx8wrapper): Prevent undefined behavior when converting Vector4 colors - #3655
Follow-up to #3651, from xezon's "Are there more of these?" on #3580. DX8Wrapper::Convert_Color(const Vector4&) and Convert_Color_Clamp pass the color to the Vector3 overload with reinterpret_cast<const Vector3&>(color). A Vector4 is not a Vector3, so reading it through that reference is undefined behavior, the same problem as #3521.
Convert_Color(const Vector4&) now builds a Vector3 from the first three components, and Convert_Color_Clamp calls it after clamping:
Disassembly: I compared the objects of five callers (dynamesh.cpp, pointgr.cpp, streakRender.cpp, W3DScorch.cpp and W3DTerrainParticles.cpp) against a build of main, for both games. In every changed function, the float arithmetic and the call targets are the same as on main.
VS 2026 (ClangCL) Release:DynamicMeshClass::Change_Vertex_Color, PointGroupClass::Render, PointGroupClass::RenderVolumeParticle and StreakRendererClass::RenderStreak changed only in register allocation and stack offsets.
VC6 Release:W3DScorch::updateScorches, the W3DTerrainParticles constructor and W3DTerrainParticles::render now copy the three floats into the Vector3 temporary (three fld/fstp pairs) before the inline assembly converts the color. The other three files are unchanged.
AI Disclosure
Claude Opus 5.5 was used to aid in implementation and to compare the disassembly.
…ctor3 cast
DX8Wrapper::Convert_Color(const Vector4&) and Convert_Color_Clamp read
the RGB part of a Vector4 through a Vector3 reference.
On VC6, the inline assembly that converts the color now lives in
Convert_Color_RGB, which takes a pointer to the red, green and blue
floats. Both Convert_Color overloads pass &color.X, so the Vector4
overload doesn't copy the color into a Vector3 temporary. Other
compilers build a Vector3 from X, Y and Z.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No actionable comments were generated in the recent review.
Walkthrough
Both DX8Wrapper Vector4 color-conversion helpers now use named X, Y, and Z components for color and W for alpha. The clamped helper still clamps a copy before conversion.
TheSuperHackers/GeneralsGameCode#3639: Adds explicit member-wise conversion helpers in WW3D2; this PR applies explicit component construction to Vector4 color conversion.
No actionable merge-blocking risk is identified in this localized color-conversion change. It is mergeable subject to normal build checks.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: it fixes undefined behavior in DX8Wrapper color conversion from Vector4. The change replaces the reinterpret_cast of a Vector4 as a Vector3 with explicit component con…
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 change in detail: DX8Wrapper::Convert_Color(const Vector4&) and Convert_Color_Clamp previously read a Vector4 through a reinterpret_cast to Vector3&, which is undefined be…
Full details: Title check
Explanation
The title names the main change: it fixes undefined behavior in DX8Wrapper color conversion from Vector4. The change replaces the reinterpret_cast of a Vector4 as a Vector3 with explicit component construction, which matches the title. The title uses a conventional prefix and runs 74 characters, which is within a reasonable length.
Full details: Description check
Explanation
The description explains the change in detail: DX8Wrapper::Convert_Color(const Vector4&) and Convert_Color_Clamp previously read a Vector4 through a reinterpret_cast to Vector3&, which is undefined behavior. It describes the replacement code, which builds a Vector3 from X, Y and Z and passes W as alpha, and the clamp path now calling the Vector4 overload. It also lists disassembly evidence for five callers across VS 2026 and VC6 builds. This matches the raw summary and the PR objectives, so the description is clearly related to the changeset.
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.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: b02a056b-7b15-45f1-a14b-88ebd8afb09d
Commits
Reviewing files that changed from the base of the PR and between 116fbcf and 273f99f.
Files selected for processing: 1
Core/Libraries/Source/WWVegas/WW3D2/dx8wrapper.h
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #3651, from xezon's "Are there more of these?" on #3580.
DX8Wrapper::Convert_Color(const Vector4&)andConvert_Color_Clamppass the color to theVector3overload withreinterpret_cast<const Vector3&>(color). AVector4is not aVector3, so reading it through that reference is undefined behavior, the same problem as #3521.Convert_Color(const Vector4&)now builds aVector3from the first three components, andConvert_Color_Clampcalls it after clamping:WWINLINE unsigned int DX8Wrapper::Convert_Color(const Vector4& color) { - return Convert_Color(reinterpret_cast<const Vector3&>(color),color[3]); + return Convert_Color(Vector3(color.X,color.Y,color.Z),color.W); } ... - return Convert_Color(reinterpret_cast<const Vector3&>(clamped_color),clamped_color[3]); + return Convert_Color(clamped_color);Evidence
dynamesh.cpp,pointgr.cpp,streakRender.cpp,W3DScorch.cppandW3DTerrainParticles.cpp) against a build ofmain, for both games. In every changed function, the float arithmetic and the call targets are the same as onmain.DynamicMeshClass::Change_Vertex_Color,PointGroupClass::Render,PointGroupClass::RenderVolumeParticleandStreakRendererClass::RenderStreakchanged only in register allocation and stack offsets.W3DScorch::updateScorches, theW3DTerrainParticlesconstructor andW3DTerrainParticles::rendernow copy the three floats into theVector3temporary (threefld/fstppairs) before the inline assembly converts the color. The other three files are unchanged.AI Disclosure
Claude Opus 5.5 was used to aid in implementation and to compare the disassembly.