refactor(w3dview): Migrate from MEMBER_ADD/MEMBER_RELEASE macros to RefCountPtr<T>

[bobtista]

• Merge after refactor(w3dview): Replace duplicate reference count macros with core library equivalents #1784

Replaces custom reference counting macros (MEMBER_ADD, MEMBER_RELEASE, SAFE_ADD_REF, SAFE_RELEASE_REF) with the type-safe RefCountPtr smart pointer from the core library.

Member Variables Migrated to RefCountPtr:

• AssetInfoClass::m_pRenderObj
• ScreenCursorClass::m_pTexture, m_pVertMaterial
• PlaySoundDialogClass::SoundObj
• SpherePropertySheetClass::m_RenderObj
• RingPropertySheetClass::m_RenderObj
• CW3DViewDoc::m_pCRenderObj, m_pCAnimation, m_pCAnimCombo, m_pCursor, m_pCursorScene
• CGraphicView::m_pCamera, m_pLightMesh

Local Variable Usage:

• Replaced MEMBER_ADD/MEMBER_RELEASE with REF_PTR_SET/REF_PTR_RELEASE for local raw pointers in 15 files

Removed Macros (Utils.h):

• MEMBER_ADD
• MEMBER_RELEASE
• SAFE_ADD_REF
• SAFE_RELEASE_REF

Notes:

• Used Create_AddRef() when taking ownership of an existing pointer
• Used Create_NoAddRef() when wrapping factory methods that return ref=1
• Added .Peek() calls when passing to functions expecting raw pointers
• Updated getter methods to return .Peek() instead of raw RefCountPtr
• Preserved all original logic and formatting

[xezon]

Before I continue review, please make another pass on this and correct all missing resource cleanups. It is important that no logic is changed, unless it fixes a bug.

[xezon]

The condition above is obsoleted.

[bobtista]

So, yes the operations inside are safe to execute regardless. If I understand correctly, the conditional checks make the intent explicit (only cleanup when there's something to clean up). RefCountPtr provides operator const DummyPtrType *() specifically to enable null-checking in boolean contexts.
// From ref_ptr.h documentation (lines 92-96):
void Do_It(void)
{
if (MyObject) { // Recommended pattern
MyObject->Do_Something();
}
}

This is also the pattern we use elsewhere eg CW3DViewDoc::CleanupResources.

Happy to keep or remove them, up to you

[xezon]

Assignment functions already do the tests and are safe.

GeneralsGameCode/Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h

Lines 289 to 298 in 7a6402b

	const RefCountPtr<T> & operator =(DummyPtrType * dummy_ptr)
	{
	if (Referent) {
	Referent->Release_Ref();
	}
	Referent = 0;
	return *this;
	}

Alternatively, use Clear() function. Maybe that is cleaner.

GeneralsGameCode/Core/Libraries/Source/WWVegas/WWLib/ref_ptr.h

Lines 352 to 358 in 7a6402b

	void Clear(void)
	{
	if (Referent) {
	Referent->Release_Ref();
	Referent = 0;
	}
	}

[bobtista]

I like Clear :) Done

[xezon]

How about we first make a change where we just remove the duplicate Macros, without adding RefCountPtr? Then this change will be simpler? And then make small changes to introduce RefCountPtr slowly? I am afraid this will be a big review here otherwise given the implications. One mistake with ref counting and the game will crash.

[bobtista]

How about we first make a change where we just remove the duplicate Macros, without adding RefCountPtr? Then this change will be simpler? And then make small changes to introduce RefCountPtr slowly? I am afraid this will be a big review here otherwise given the implications. One mistake with ref counting and the game will crash.

Ok, check out 1784. Shall we continue this PR after rebasing and have it focus just on just W3DView, then additional RefCountPtr migrations can have separate PRs?

[xezon]

This still needs work.

[xezon]

This refactor needs another pass. Something went wrong the indentation of code. And behavior has changed.

[xezon]

m_pCBackObjectScene.Clear() missing?

[bobtista]

m_pCBackObjectScene is a raw pointer (SceneClass *), not a RefCountPtr, so it uses Release_Ref() and = NULL for cleanup (lines 167-168). Want me to convert it to RefCountPtr? Or should that be in a separate PR?

[bobtista]

Checking again - On main:
m_pCursorScene used REF_PTR_RELEASE and was migrated to RefCountPtr with .Clear().
m_pCBackObjectScene used manual Release_Ref() + = NULL (not REF_PTR_RELEASE), so it wasn’t done in this PR. Do you want me to migrate it in this PR?

[bobtista]

This indentation seems wrong in the original code, fixed here

[bobtista]

I think this is ready for another look

[bobtista]

Here too, the indentation was weird in the original. It looks like they meant to do:
Function body level: 4 spaces
Inside the if block: 8 spaces (4 + 4)
But a few spots got 10 spaces, eg SAFE_DELETE, so I fixed those, at least here

[greptile-apps]

Greptile Summary

This PR replaces manual reference-counting macros (MEMBER_ADD, MEMBER_RELEASE, SAFE_ADD_REF, SAFE_RELEASE_REF, REF_PTR_SET, REF_PTR_RELEASE) across the W3DView tool with the type-safe RefCountPtr<T> smart pointer, eliminating whole classes of raw-pointer misuse while preserving all original logic.

• Member variables across 8 classes (m_pRenderObj, m_pCamera, m_pLightMesh, m_pCursorScene, m_pCRenderObj, m_pCAnimation, m_pCursor, SoundObj, m_pTexture, m_pVertMaterial) migrated to RefCountPtr<T> with correctly chosen Create_AddRef (existing objects) vs Create_NoAddRef (factory/new returns).
• Destructor and cleanup boilerplate removed where RefCountPtr now handles lifetime automatically; .Clear() used in CleanupResources() where an explicit early release is needed.
• Constructor initializer-list order for SpherePropertySheetClass and RingPropertySheetClass corrected to place the base class (CPropertySheet) before member initializers, matching C++ initialization order and eliminating a latent compiler warning.

Confidence Score: 5/5

Safe to merge — a mechanical but carefully executed refactoring with no logic changes.

All Create_AddRef / Create_NoAddRef choices are consistent with each call site's ownership contract. The removed cleanup block for m_pCBackObjectScene after the dazzle-layer deletion was dead code (the earlier if-block in CleanupResources already nulled the pointer). AssetInfo::Initialize()'s manual Add_Ref / REF_PTR_RELEASE pattern on the local prender_obj is preserved and balances correctly in both the wrapped-instance and the factory-temporary paths. Constructor initializer-list reordering for the property sheets aligns with C++ initialization order without changing runtime behavior.

No files require special attention.

Important Files Changed

Filename	Overview
Core/Tools/W3DView/W3DViewDoc.cpp	Largest file in the PR; migrates m_pCRenderObj, m_pCAnimation, m_pCursorScene, and m_pCursor to RefCountPtr with correct Create_NoAddRef/Create_AddRef choices. The removed redundant m_pCBackObjectScene block after m_pDazzleLayer was dead code (the earlier if-block already nulled it). Cleanup, OnNewDocument, and DisplayObject paths all preserve original semantics.
Core/Tools/W3DView/W3DViewDoc.h	m_pCRenderObj, m_pCAnimation, m_pCursorScene, and m_pCursor declaration types updated to RefCountPtr; accessor methods updated to return .Peek().
Core/Tools/W3DView/AssetInfo.cpp	Initialize() updated to use .Peek(); the Add_Ref/REF_PTR_RELEASE pattern on the local prender_obj is preserved and correct — it balances the ref on both the wrapped-instance and the factory-created temporary paths.
Core/Tools/W3DView/AssetInfo.h	m_pRenderObj migrated to RefCountPtr; Get_Render_Obj and Peek_Render_Obj updated to use .Peek() with correct manual Add_Ref semantics preserved in Get_Render_Obj.
Core/Tools/W3DView/GraphicView.cpp	m_pCamera and m_pLightMesh migrated to RefCountPtr with correct Create_NoAddRef for newly allocated objects; .Peek() used consistently for raw pointer API calls.
Core/Tools/W3DView/ScreenCursor.cpp	m_pTexture and m_pVertMaterial migrated to RefCountPtr. The copy constructor does not copy these fields (it calls Initialize() instead), which matches the original behavior where they were explicitly initialized to nullptr and rebuilt.
Core/Tools/W3DView/RingPropertySheet.cpp	m_RenderObj migrated to RefCountPtr; constructor initializer list reordered to put base class first (matching C++ initialization order), fixing a latent warning. Create_AddRef used for existing objects, Create_NoAddRef for newly constructed RingRenderObjClass.
Core/Tools/W3DView/SpherePropertySheet.cpp	Parallel refactoring to RingPropertySheet — identical pattern, identical correctness. Base class reordering and Create_AddRef/Create_NoAddRef choices are both right.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph Before["Before (Manual Macros)"]
        B1["Raw pointer member\n(e.g. RenderObjClass* m_pCRenderObj)"] --> B2["Constructor: m_pRenderObj = nullptr\nthen REF_PTR_SET adds ref"]
        B2 --> B3["Usage: pass raw pointer directly"]
        B3 --> B4["Destructor/Cleanup:\nREF_PTR_RELEASE or\nRelease_Ref + nullptr"]
    end

    subgraph After["After (RefCountPtr)"]
        A1["Smart pointer member\n(e.g. RefCountPtr<RenderObjClass> m_pCRenderObj)"] --> A2["Factory result → Create_NoAddRef\nExisting object → Create_AddRef"]
        A2 --> A3["Usage: .Peek() for raw pointer APIs\nbool/== works directly"]
        A3 --> A4["Destructor: automatic\nEarly release: .Clear()"]
    end

    Before --> |Refactored to| After

Loading

%%{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
    subgraph Before["Before (Manual Macros)"]
        B1["Raw pointer member\n(e.g. RenderObjClass* m_pCRenderObj)"] --> B2["Constructor: m_pRenderObj = nullptr\nthen REF_PTR_SET adds ref"]
        B2 --> B3["Usage: pass raw pointer directly"]
        B3 --> B4["Destructor/Cleanup:\nREF_PTR_RELEASE or\nRelease_Ref + nullptr"]
    end

    subgraph After["After (RefCountPtr)"]
        A1["Smart pointer member\n(e.g. RefCountPtr<RenderObjClass> m_pCRenderObj)"] --> A2["Factory result → Create_NoAddRef\nExisting object → Create_AddRef"]
        A2 --> A3["Usage: .Peek() for raw pointer APIs\nbool/== works directly"]
        A3 --> A4["Destructor: automatic\nEarly release: .Clear()"]
    end

    Before --> |Refactored to| After

Loading

_{Reviews (2): Last reviewed commit: "refactor(w3dview): Migrate W3DViewDoc an..." | Re-trigger Greptile}

[bobtista]

Ready for re-review. Rebased onto latest main and it's behavior-consistent with the original.

Note - these fixes can either be added here or to separate PRs if we want:

1. m_pCursor.Clear() in CleanupResources - pulled this back out so the cursor is only released when the document is destroyed (as before, now via the RefCountPtr destructor). Clearing it in CleanupResources would release/recreate the cursor on every cleanup (e.g. File→New), which is a behavior change.
2. Leak in DisplayObject (ghost-object branch) - there's a local RenderObjClass *m_pCRenderObj that shadows the member; it's Add_Ref()'d and added to the scene but never released, so the local's reference leaks. Left as-is here to keep behavior identical.
3. GraphicView::OnDestroy - kept the explicit m_pCamera/m_pLightMesh release. With RefCountPtr these could release via the destructor instead, but that shifts release timing off WM_DESTROY, so I left the explicit clears.