Skip to content

[GEN][ZH] Change reference counter implementation for StateMachine to avoid mismatch with Retail #1078

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xezon merged 6 commits into from
Jun 17, 2025
Merged

[GEN][ZH] Change reference counter implementation for StateMachine to avoid mismatch with Retail #1078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a03e7e3
[ZH] Change reference counter implementation for StateMachine to avoi…
xezon Jun 17, 2025
3843801
Simplify constness for caller of RefCountValue::Release_Ref
xezon Jun 17, 2025
1fe749e
Fix VC6 compile error
xezon Jun 17, 2025
d4459c9
Fix wrong assert condition
xezon Jun 17, 2025
6e69c4f
Replicate in Generals
xezon Jun 17, 2025
1885c3d
Add a bunch of comments
xezon Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 67 additions & 2 deletions Core/Libraries/Source/WWVegas/WWLib/refcount.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ struct ActiveRefStruct
typedef DataNode<RefCountClass *> RefCountNodeClass;
typedef List<RefCountNodeClass *> RefCountListClass;

/*
** Note that Add_Ref and Release_Ref are always const, because copying, destroying and reference
** counting const objects is meant to work.
*/
class RefCountClass
{
public:
Expand Down Expand Up @@ -249,10 +253,71 @@ class RefCountClass
*/
static bool Validate_Active_Ref(RefCountClass * obj);

#endif
#endif // NDEBUG

};


/*
** This template class is meant to be used as a class member for compact reference counter placements.
** A 1 byte reference counter can be alright if the counter is not reaching the value limits.
*
** Note that Add_Ref and Release_Ref are always const, because copying, destroying and reference
** counting const objects is meant to work.
*/
template <typename IntegerType>
class RefCountValue
{
public:

#endif
RefCountValue(void)
: NumRefs(1)
{
}

~RefCountValue(void)
{
WWASSERT(NumRefs == IntegerType(0));
}

/*
** Add_Ref, call this function if you are going to keep a pointer to this object.
*/
void Add_Ref(void) const
{
WWASSERT(NumRefs != ~IntegerType(0));
++NumRefs;
}

/*
** Release_Ref, call this function when you no longer need the pointer to this object.
** You can pass a static function of type void(*)(DeleteType*) or 'operator delete'.
**
** Note that this function takes a const ObjectType*, because this function is expected
** to be called from within a const function as well.
*/
template <typename DeleteFunction, typename ObjectType>
void Release_Ref(DeleteFunction deleteFunction, const ObjectType* objectToDelete) const
{
WWASSERT(NumRefs != IntegerType(0));
if (--NumRefs == IntegerType(0))
{
deleteFunction(const_cast<ObjectType*>(objectToDelete));

@Caball009 Caball009 Jun 17, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of having a const function and mutable member variable? I see the consistency with RefCountClass, but I don't see why it's needed there either.

@xezon xezon Jun 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required so that const objects can still do reference counting. Otherwise we cannot pass around const objects, or destroy them.

@Caball009 Caball009 Jun 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense if those const objects use a mutable RefCountValue member variable?

@xezon xezon Jun 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The const_cast needs to be somewhere. I think it is better if it is buried here in this class and not in all code that interacts with this class.

@Caball009 Caball009 Jun 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the const_cast need to be anywhere if the RefCountValue is mutable?

struct RefCountValue
{
	int NumRefs = 0;

	void Release_Ref()
	{
		--NumRefs;
	}
};

struct StateMachine
{
	void Release_Ref() const
	{
		m_refCount.Release_Ref();
	}

	mutable RefCountValue m_refCount;
};

int main()
{
	const StateMachine machine;
	machine.Release_Ref();
}

Is this not a representative minimal example?

@xezon xezon Jun 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, const cast happens when passing this pointer to delete function.

@Caball009 Caball009 Jun 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Maybe you can add a comment for RefCountValue::Release_Ref that explains the use of const member function and const_cast on the object that'll be deleted.

@xezon xezon Jun 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Added a bunch of comments in EA/Westwood comment style.

}
}

/*
** Check the number of references to this object.
*/
IntegerType Num_Refs(void) const
{
return NumRefs;
}

private:

mutable IntegerType NumRefs;
};


#endif // REFCOUNT_H
14 changes: 7 additions & 7 deletions Generals/Code/GameEngine/Include/Common/StateMachine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ inline State::~State() { }
/**
* A finite state machine.
*/
class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountClass
class StateMachine : public MemoryPoolObject, public Snapshot
{
MEMORY_POOL_GLUE_WITH_USERLOOKUP_CREATE( StateMachine, "StateMachinePool" );

Expand All @@ -264,6 +264,10 @@ class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountCl

virtual StateReturnType setState( StateID newStateID ); ///< change the current state of the machine (which may cause further state changes, due to onEnter)

void Add_Ref() const { m_refCount.Add_Ref(); }
void Release_Ref() const { m_refCount.Release_Ref(MemoryPoolObject::deleteInstanceInternal, this); }
void Num_Refs() const { m_refCount.Num_Refs(); }

StateID getCurrentStateID() const { return m_currentState ? m_currentState->getID() : INVALID_STATE_ID; } ///< return the id of the current state of the machine
Bool isInIdleState() const { return m_currentState ? m_currentState->isIdle() : true; } // stateless things are considered 'idle'
Bool isInAttackState() const { return m_currentState ? m_currentState->isAttack() : true; } // stateless things are considered 'idle'
Expand Down Expand Up @@ -342,12 +346,6 @@ class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountCl
virtual void xfer( Xfer *xfer );
virtual void loadPostProcess();

// RefCountClass interface
virtual void Delete_This()
{
MemoryPoolObject::deleteInstanceInternal(this);
}

protected:

/**
Expand Down Expand Up @@ -384,6 +382,8 @@ class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountCl
Bool m_locked; ///< whether this machine is locked or not
Bool m_defaultStateInited; ///< if initDefaultState has been called

RefCountValue<UnsignedByte> m_refCount;

#ifdef STATE_MACHINE_DEBUG
Bool m_debugOutput;
AsciiString m_name; ///< Human readable name of this state - for debugging. jba.
Expand Down
14 changes: 7 additions & 7 deletions GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ inline State::~State() { }
/**
* A finite state machine.
*/
class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountClass
class StateMachine : public MemoryPoolObject, public Snapshot
{
MEMORY_POOL_GLUE_WITH_USERLOOKUP_CREATE( StateMachine, "StateMachinePool" );

Expand All @@ -265,6 +265,10 @@ class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountCl

virtual StateReturnType setState( StateID newStateID ); ///< change the current state of the machine (which may cause further state changes, due to onEnter)

void Add_Ref() const { m_refCount.Add_Ref(); }
void Release_Ref() const { m_refCount.Release_Ref(MemoryPoolObject::deleteInstanceInternal, this); }
Comment thread
xezon marked this conversation as resolved.
void Num_Refs() const { m_refCount.Num_Refs(); }

StateID getCurrentStateID() const { return m_currentState ? m_currentState->getID() : INVALID_STATE_ID; } ///< return the id of the current state of the machine
Bool isInIdleState() const { return m_currentState ? m_currentState->isIdle() : true; } // stateless things are considered 'idle'
Bool isInAttackState() const { return m_currentState ? m_currentState->isAttack() : true; } // stateless things are considered 'idle'
Expand Down Expand Up @@ -344,12 +348,6 @@ class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountCl
virtual void xfer( Xfer *xfer );
virtual void loadPostProcess();

// RefCountClass interface
virtual void Delete_This()
{
MemoryPoolObject::deleteInstanceInternal(this);
}

protected:

/**
Expand Down Expand Up @@ -386,6 +384,8 @@ class StateMachine : public MemoryPoolObject, public Snapshot, public RefCountCl
Bool m_locked; ///< whether this machine is locked or not
Bool m_defaultStateInited; ///< if initDefaultState has been called

RefCountValue<UnsignedByte> m_refCount;

#ifdef STATE_MACHINE_DEBUG
Bool m_debugOutput;
AsciiString m_name; ///< Human readable name of this state - for debugging. jba.
Expand Down
Loading