refactor(sphere): Zero initialize SphereClass by default and add IsEmpty function#2855
refactor(sphere): Zero initialize SphereClass by default and add IsEmpty function#2855stephanmeesters wants to merge 3 commits into
Conversation
|
| Filename | Overview |
|---|---|
| Core/Libraries/Source/WWVegas/WWMath/sphere.h | Core change: default constructor now zero-initializes Center and Radius; IsEmpty() added and used in Add_Sphere/Add_Spheres; verbose EA comment blocks removed. Logic is sound. |
| Core/Libraries/Source/WWVegas/WW3D2/collect.cpp | Replaces explicit SphereClass(Vector3(0,0,0),0) with SphereClass() — behaviour identical given the new default constructor. |
| Core/Libraries/Source/WWVegas/WW3D2/dynamesh.cpp | Trivial call-site cleanup: SphereClass(Vector3(0,0,0),0) → SphereClass(). |
| Core/Libraries/Source/WWVegas/WW3D2/sortingrenderer.cpp | Inline temporary SphereClass() replaces named zero-initialized variable; no behaviour change. |
| Core/Tools/W3DView/ViewerScene.cpp | SphereClass accumulator switches to default constructor; functionally identical. |
| Generals/Code/Libraries/Source/WWVegas/WW3D2/boxrobj.cpp | Removes an unused SphereClass variable and Get_Obj_Space_Bounding_Sphere call whose result was never read — clean dead code removal. |
| Generals/Code/Libraries/Source/WWVegas/WW3D2/dazzle.cpp | Three unused SphereClass variables (never read after construction) removed in Render_Dazzle. |
| Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/Shadow/W3DVolumetricShadow.cpp | Removes declared-but-unused SphereClass bsphere variable in renderShadows. |
| GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/boxrobj.cpp | Mirror of Generals boxrobj.cpp: same unused SphereClass variable removed. |
| GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/dazzle.cpp | Mirror of Generals dazzle.cpp: three unused SphereClass variables removed. |
| GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/Shadow/W3DVolumetricShadow.cpp | Mirror of Generals W3DVolumetricShadow.cpp: unused bsphere variable removed. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SphereClass default ctor\nbefore: uninitialized] -->|PR change| B[SphereClass default ctor\nCenter=0,0,0 Radius=0]
B --> C[IsEmpty: Radius <= 0.0]
C -->|used in| D[Add_Sphere\nold: s.Radius == 0.0f\nnew: s.IsEmpty]
C -->|used in| E[Add_Spheres\nold: s0.Radius == 0.0f\nnew: s0.IsEmpty]
F[Call-site cleanup] --> G[collect.cpp\ndynamesh.cpp\nsortingrenderer.cpp\nViewerScene.cpp\nuse SphereClass instead of\nSphereClass Vector3 0 0 0 0]
H[Dead code removal] --> I[boxrobj.cpp x2\ndazzle.cpp x2\nW3DVolumetricShadow.cpp x2\nunused SphereClass variables\nremoved]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[SphereClass default ctor\nbefore: uninitialized] -->|PR change| B[SphereClass default ctor\nCenter=0,0,0 Radius=0]
B --> C[IsEmpty: Radius <= 0.0]
C -->|used in| D[Add_Sphere\nold: s.Radius == 0.0f\nnew: s.IsEmpty]
C -->|used in| E[Add_Spheres\nold: s0.Radius == 0.0f\nnew: s0.IsEmpty]
F[Call-site cleanup] --> G[collect.cpp\ndynamesh.cpp\nsortingrenderer.cpp\nViewerScene.cpp\nuse SphereClass instead of\nSphereClass Vector3 0 0 0 0]
H[Dead code removal] --> I[boxrobj.cpp x2\ndazzle.cpp x2\nW3DVolumetricShadow.cpp x2\nunused SphereClass variables\nremoved]
Reviews (2): Last reviewed commit: "Remove unused instances of SphereClass" | Re-trigger Greptile
Caball009
left a comment
There was a problem hiding this comment.
While looking at instances of SphereClass I noticed a couple of places where they're unused:
GeneralsGameCode/GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/boxrobj.cpp
Lines 514 to 516 in 838f9d0
I realize it's slightly outside the scope of this PR, but perhaps these can be removed as well.
Caball009
left a comment
There was a problem hiding this comment.
I don't really have an opinion on the removal of the EA / Westwood comments, but the code changes look fine to me.
| * operator + -- Add two spheres together, creating a sphere which encloses both * | ||
| * Transform Sphere -- transform a sphere * | ||
| * Transform_Sphere -- transform a sphere * | ||
| * operator * -- Transform a sphere * |
There was a problem hiding this comment.
I think removing these comments is too much for this change. Many files in WW Vegas have these comments.
There was a problem hiding this comment.
That's fair but I'd like not adding more of it, nor updating that list of functions with descriptions
There was a problem hiding this comment.
Indeed these comments are quite mad. Maybe we should make a general pass and purge them or simplify them? I think preserving some of the descriptions is ok, but the author and dates and big blocks of symbols are annoying.
| inline void Add_Sphere(const SphereClass & s); | ||
| inline void Transform(const Matrix3D & tm); | ||
| inline float Volume() const; | ||
| inline bool IsEmpty() const; |
There was a problem hiding this comment.
Naming convention for WW Vegas functions is Is_Empty
| inline void Add_Sphere(const SphereClass & s); | ||
| inline void Transform(const Matrix3D & tm); | ||
| inline float Volume() const; | ||
| inline bool IsEmpty() const; |
There was a problem hiding this comment.
Does this function name make sense for this geometry? An empty sphere is a sphere with zero radius? Chat gippy says:
In standard geometry, a sphere with radius 0 is called a degenerate sphere. It consists of exactly one point—its center.
You can think of it this way:
- For (r > 0), a sphere is the set of all points at distance (r) from a center.
- If (r = 0), the only point whose distance from the center is 0 is the center itself.
So the sphere "collapses" to a single point.
In different contexts, you might hear:
- Degenerate sphere (most common in geometry)
- Point sphere (occasionally used)
- Simply a point, especially in topology or analysis, since the set is ({c}) for center (c).
The same idea applies to other shapes:
- A circle of radius 0 degenerates to a point.
- A line segment of length 0 degenerates to a point.
- A triangle whose vertices are all the same point is a degenerate triangle.
So, the most precise mathematical term is degenerate sphere, though in many proofs and discussions mathematicians will simply say "the sphere of radius 0," understanding that it is the singleton set containing the center.
There was a problem hiding this comment.
How about Is_Valid? It's basically uninitialized or invalid if radius <= 0
| inline void Add_Sphere(const SphereClass & s); | ||
| inline void Transform(const Matrix3D & tm); | ||
| inline float Volume() const; | ||
| inline bool IsEmpty() const; |
There was a problem hiding this comment.
And sphere with radius < 0:
In standard geometry, there is no such thing as a sphere with a negative radius. By definition, a radius is a non-negative distance, so it satisfies
[
r \ge 0.
]
If someone writes (r < 0), there are a few possibilities:
-
It's an invalid sphere. This is the usual mathematical interpretation.
-
It's shorthand or an intermediate calculation. For example, an equation solver may return (r = \pm 5), but only (r = 5) is accepted as the radius of a sphere.
-
It's being used in a generalized context. In some areas of mathematics and computer graphics, a "signed radius" may be used as a convention. For example:
- In signed distance fields (SDFs), a negative radius can represent the complement or an "inverted" sphere.
- In oriented geometry, a negative radius may indicate an orientation rather than a different geometric object.
Geometrically, however, a sphere of radius (-r) is identical to one of radius (r), because the defining equation is
[
(x-a)^2 + (y-b)^2 + (z-c)^2 = r^2.
]
Since ((-r)^2 = r^2), the set of points is exactly the same. The sign disappears when squaring.
So unlike a radius of 0 (which gives a degenerate sphere consisting of a single point), a negative radius does not define a new kind of sphere in classical geometry. It is either invalid or a signed quantity used by convention in a specialized field.
SphereClassto zero-init its members.IsEmptyfunctionSphereClassif merged should be done after #2854