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

bobtista · 2025-10-31T00:51:54Z

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.

Core/Tools/W3DView/W3DViewDoc.cpp

xezon · 2025-11-02T16:38:23Z

Core/Tools/W3DView/W3DViewDoc.cpp


    // Was there a valid display object?
    if (m_pCRenderObj)
    {


The condition above is obsoleted.

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

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;

}

}

I like Clear :) Done

Core/Tools/W3DView/W3DViewDoc.h

…o RefCountPtr<T> Replace MEMBER_ADD/MEMBER_RELEASE macros with RefCountPtr<T> smart pointers. - AssetInfo: m_pRenderObj now RefCountPtr<RenderObjClass> - ScreenCursor: m_pTexture and m_pVertMaterial now RefCountPtr - PlaySoundDialog: SoundObj now RefCountPtr<AudibleSoundClass>

Replace MEMBER_ADD/MEMBER_RELEASE macros with RefCountPtr<T> smart pointers. - SpherePropertySheet: m_RenderObj now RefCountPtr<SphereRenderObjClass> - RingPropertySheet: m_RenderObj now RefCountPtr<RingRenderObjClass> - EmitterPropertySheet: local emitter variables use REF_PTR_RELEASE - EmitterInstanceList: use REF_PTR_RELEASE for vector cleanup

Replace MEMBER_ADD/MEMBER_RELEASE macros with RefCountPtr<T> and REF_PTR_RELEASE. - W3DViewDoc: m_pCRenderObj, m_pCAnimation, m_pCursorScene, m_pCursor now RefCountPtr - GraphicView: m_pCamera and m_pLightMesh now RefCountPtr - Local variables in Utils, DataTreeView, BoneMgrDialog, etc. use REF_PTR_RELEASE

Remove obsolete macros now replaced by RefCountPtr<T> and core library macros. - Removed: MEMBER_ADD, MEMBER_RELEASE, SAFE_ADD_REF, SAFE_RELEASE_REF - Kept: SAFE_DELETE, SAFE_DELETE_ARRAY, COM_RELEASE, SAFE_CLOSE, SANITY_CHECK

…rScene in CleanupResources

… checks

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.

xezon · 2025-11-02T21:07:48Z

Core/Tools/W3DView/W3DViewDoc.cpp

 		m_pCursor->Remove ();
 	}
-	MEMBER_RELEASE (m_pCursorScene);
+	m_pCursor.Clear();


This clear is wrong.

xezon · 2025-11-02T21:09:26Z

Core/Tools/W3DView/W3DViewDoc.cpp

+			// Remove this object from the scene
+			Remove_Object_From_Scene (m_pCRenderObj.Peek());
+		}
+		m_pCRenderObj.Clear();


This can be moved into scope above.

xezon · 2025-11-02T21:10:24Z

Core/Tools/W3DView/W3DViewDoc.cpp

-					m_pCRenderObj = NULL;
+					Remove_Object_From_Scene (m_pCRenderObj.Peek());
 			  }
+			  m_pCRenderObj.Clear();


This can be moved into scope above

xezon · 2025-11-02T21:10:33Z

Core/Tools/W3DView/W3DViewDoc.cpp

    {
        // Lose the animation
 		  SAFE_DELETE (m_pCAnimCombo);
-		  MEMBER_RELEASE (m_pCAnimation);


This is wrong.

xezon · 2025-11-02T21:11:47Z

Core/Tools/W3DView/W3DViewDoc.cpp

-		  MEMBER_RELEASE (m_pCAnimation);
-        m_pCAnimation = WW3DAssetManager::Get_Instance()->Get_HAnim (pszAnimationName);
+        m_pCAnimation = RefCountPtr<HAnimClass>::Create_NoAddRef(WW3DAssetManager::Get_Instance()->Get_HAnim (pszAnimationName));
        ASSERT (m_pCAnimation);


.Peek()

Multiple times in this change. Please check what's up with ASSERT.

bobtista · 2025-11-03T00:40:18Z

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?

bobtista force-pushed the bobtista/refactor-w3dview-refcountptr branch 8 times, most recently from cc1d8dc to c3f8d36 Compare October 31, 2025 02:22

Skyaero42 added Refactor Edits the code with insignificant behavior changes, is never user facing Stability Concerns stability of the runtime labels Nov 1, 2025

xezon reviewed Nov 2, 2025

View reviewed changes

bobtista force-pushed the bobtista/refactor-w3dview-refcountptr branch from c3f8d36 to 0cbd2fc Compare November 2, 2025 18:27

bobtista added 6 commits November 2, 2025 13:28

fix(w3dview): add missing NULL assignments for m_pCursor and m_pCurso…

a402257

…rScene in CleanupResources

nit: revert indentation change

4c2acf0

bobtista force-pushed the bobtista/refactor-w3dview-refcountptr branch from 0cbd2fc to 4c2acf0 Compare November 2, 2025 18:28

refactor(w3dview): use RefCountPtr::Clear() and remove redundant null…

3c0d862

… checks

xezon reviewed Nov 2, 2025

View reviewed changes

bobtista mentioned this pull request Nov 3, 2025

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

Open

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

	Referent = 0;

	return *this;
	}

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

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

Are you sure you want to change the base?

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

Conversation

bobtista commented Oct 31, 2025 • edited by xezon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobtista commented Nov 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bobtista commented Oct 31, 2025 •

edited by xezon

Loading