Custom Ref Counting

[The3dVehicleguy]

Massive refactor with significant changes to the way smart pointer and resource management system operates, transitioning from standard C++ shared_ptr/weak_ptr to custom Ref, WeakRef, and related utilities. It also introduces a new RefCountTracker system for detailed reference counting and leak detection (in debug/profile builds), and updates the command pattern implementation to use the new reference types.

The most important changes are:

Reference Counting and Smart Pointer System

• Introduced a new file, RefCountTracker.h, which implements a comprehensive reference counting tracker for all RefCounted objects, including statistics, leak detection, and optional Tracy profiler integration. Macros are provided to enable or disable tracking based on build configuration.
• Updated various usages of shared_ptr/weak_ptr throughout the editor and runtime code to use the new Ref, WeakRef, and related methods (Get(), Lock(), Reset(), etc.), ensuring consistent usage of the custom smart pointer system. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]

Command Pattern Refactoring

• Made Command inherit from RefCounted and refactored the CommandStack to use Ref<Command> instead of shared_ptr<Command>. Updated all related buffer and stack logic to use the new smart pointer types and creation utilities (CreateRef). [1] [2] [3] [4] [5] [6]

API Consistency and Naming

• Updated function signatures and member variables to use the new reference types (Ref, WeakRef) and replaced standard pointer method calls (.get(), .lock(), .reset()) with the new API (.Get(), .Lock(), .Reset(), .Expired()). [1] [2] [3] [4]

These changes modernize and unify the project's memory management approach, improve debugging and profiling capabilities, and lay the groundwork for safer and more maintainable code.

[Copilot]

Pull request overview

Transitions the engine/editor from std::shared_ptr/std::weak_ptr to a custom reference-counted smart pointer system (Ref, WeakRef, Scope) and introduces optional reference-count tracking for debugging/profiling.

Changes:

• Added new RefCounted/Ref/WeakRef/Scope utilities (plus optional tracking hooks) and wired them into the precompiled header.
• Replaced many shared_ptr/weak_ptr usages across runtime/editor systems (resources, rendering, world/components, commands) with Ref/WeakRef.
• Updated a few misc APIs to C++20 utilities (e.g., std::ranges::sort, std::cmp_not_equal) alongside the refactor.

Reviewed changes

Copilot reviewed 35 out of 36 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
source/runtime/World/World.cpp	Switch resource save/load codepaths from shared_ptr to Ref.
source/runtime/World/Entity.h	Convert component storage/creation to Ref (CreateRef, .As<>, .Get()).
source/runtime/World/Entity.cpp	Update component iteration to use Ref containers.
source/runtime/World/Components/Terrain.h	Convert terrain resource members to Ref and adjust includes accordingly.
source/runtime/World/Components/Terrain.cpp	Replace make_shared/.get() with CreateRef/.Get() for terrain assets.
source/runtime/World/Components/Renderable.h	Change material/instance buffer handling to Ref.
source/runtime/World/Components/Renderable.cpp	Update resource lookups/caching and GPU buffer creation to use Ref.
source/runtime/Resource/ResourceCache.h	Convert cache APIs and internal storage to Ref types.
source/runtime/Resource/ResourceCache.cpp	Convert cache internals, lookups, and icon storage to Ref.
source/runtime/Resource/Import/ModelImporter.cpp	Update material/texture loading signatures and locals to Ref.
source/runtime/Rendering/Renderer_Resources.cpp	Convert renderer-owned resources (states, RTs, shaders, buffers, std assets) to Ref.
source/runtime/Rendering/Renderer_Passes.cpp	Update command list binding to use .Get() from Ref buffers.
source/runtime/Rendering/Renderer.h	Update renderer accessors and member types to Ref/Scope.
source/runtime/Rendering/Renderer.cpp	Update renderer globals and font access to Ref.
source/runtime/Rendering/Material.h	Update texture setter overload to accept Ref<RHI_Texture>.
source/runtime/Rendering/Material.cpp	Replace shared_ptr texture/material flows with Ref equivalents.
source/runtime/RHI/Vulkan/Vulkan_Device.cpp	Update bindless sampler update API to accept Ref samplers.
source/runtime/RHI/RHI_Device.h	Update bindless sampler parameter type to Ref array.
source/runtime/Geometry/Mesh.h	Replace internal GPU buffer ownership from unique_ptr to Scope.
source/runtime/Game/Game.cpp	Update game/world setup to create and pass meshes/materials/textures via Ref.
source/runtime/Display/Display.cpp	Use C++20 std::ranges::sort and std::cmp_not_equal in display mode logic.
source/runtime/Core/pch.h	Add RefCounter.h to PCH to make new ref utilities widely available.
source/runtime/Core/ThreadPool.cpp	Minor formatting change in task packaging line.
source/runtime/Core/SpartanObject.h	Make SpartanObject inherit from RefCounted.
source/runtime/Core/RefCounter.h	Add new custom smart pointer/refcount implementation (Ref, WeakRef, Scope).
source/runtime/Core/RefCountTracker.h	Add optional refcount tracking/leak detection (debug/profile).
source/runtime/Commands/CommandStack.h	Convert command stack storage and creation to Ref<Command>.
source/runtime/Commands/CommandStack.cpp	Convert undo/redo buffers and optionals to Ref.
source/runtime/Commands/Command.h	Make Command inherit from RefCounted.
source/editor/Widgets/TextureViewer.cpp	Update render-target enumeration to use Ref from renderer.
source/editor/Widgets/ShaderEditor.cpp	Update shader enumeration to use Ref.
source/editor/Widgets/ResourceViewer.cpp	Update resource listing/sorting to use Ref.
source/editor/Widgets/Properties.h	Update inspected material handle from weak_ptr to WeakRef.
source/editor/Widgets/Properties.cpp	Update material inspection flow to use WeakRef API.
source/editor/Widgets/FileDialog.cpp	Update thumbnail caching to use .Get() from Ref.
source/editor/ImGui/ImGui_Extension.h	Update drag/drop texture loading to use .Get() from Ref.

Comments suppressed due to low confidence (1)

source/runtime/Core/SpartanObject.h:36

• SpartanObject now derives from RefCounted, but this header doesn't include the definition of RefCounted (nor <chrono> despite using std::chrono::high_resolution_clock). This makes the header non-self-contained and can break compilation depending on include order/PCH usage. Include the appropriate headers directly (e.g., Core/RefCounter.h and <chrono>).

namespace spartan
{
    class SpartanObject : public RefCounted 
    {
    public:
        SpartanObject()
        {
            // stack-only, deterministic pseudo-random ID
            auto time_now = static_cast<uint64_t>(std::chrono::high_resolution_clock::now().time_since_epoch().count());

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The explicit template instantiation definitions (template class Ref<RefCounted>; / template class WeakRef<RefCounted>;) are in a header that is included widely (via pch.h). Explicit instantiation definitions must appear in exactly one translation unit; leaving them in the header will typically cause ODR/multiple-definition linker errors. Move these to a single .cpp file, or change them to extern template declarations in the header with one corresponding definition in a .cpp.

Suggested change

	* @brief Explicit template instantiation for Ref<RefCounted>
	*
	* This explicit instantiation ensures that the compiler generates all the code
	* for Ref<RefCounted> at this point, making it available to all translation units
	* that include this header without having to recompile the template for each use.
	*
	* RefCounted is the base class for all reference-counted objects in the system,
	* so this instantiation is particularly important for the smart pointer system.
	*/
	template class Ref<RefCounted>;
	/**
	* @brief Explicit template instantiation for WeakRef<RefCounted>
	*
	* This explicit instantiation ensures that the compiler generates all the code
	* for WeakRef<RefCounted> at this point, making it available to all translation units
	* that include this header without having to recompile the template for each use.
	*
	* Weak references to RefCounted objects allow tracking objects without preventing their
	* deletion when all strong references (Ref<T>) are gone, which is essential for
	* breaking reference cycles and implementing observer patterns.
	*/
	template class WeakRef<RefCounted>;
	* @brief Explicit template instantiation declaration for Ref<RefCounted>
	*
	* This extern explicit instantiation declaration tells the compiler that the
	* instantiation of Ref<RefCounted> is provided in a single translation unit,
	* avoiding re-instantiation in every translation unit that includes this header.
	*
	* RefCounted is the base class for all reference-counted objects in the system,
	* so this instantiation is particularly important for the smart pointer system.
	*/
	extern template class Ref<RefCounted>;
	/**
	* @brief Explicit template instantiation declaration for WeakRef<RefCounted>
	*
	* This extern explicit instantiation declaration tells the compiler that the
	* instantiation of WeakRef<RefCounted> is provided in a single translation unit,
	* avoiding re-instantiation in every translation unit that includes this header.
	*
	* Weak references to RefCounted objects allow tracking objects without preventing their
	* deletion when all strong references (Ref<T>) are gone, which is essential for
	* breaking reference cycles and implementing observer patterns.
	*/
	extern template class WeakRef<RefCounted>;

[Copilot]

ResourceCache now derives from RefCounted, but ResourceCache.h does not include the definition of RefCounted. Relying on indirect includes/PCH makes the header fragile. Include Core/RefCounter.h (or otherwise ensure RefCounted is defined) before using it as a base class.

[Copilot]

Ref tracking currently reports every object as type RefCounted because the base-class constructors call SPARTAN_TRACK_REFCOUNT_CREATE(this, RefCounted). This prevents per-derived-type statistics/leak reporting, which seems to conflict with the stated goal of tracking "all RefCounted objects" with detailed stats by type. Consider recording typeid(*this).name() (or providing a required macro/CRTP hook in derived constructors) so the tracker gets the concrete type name.

[Copilot]

RefCountTracker::GetStats() returns RefCountStats by value, but RefCountStats contains std::atomic members which are non-copyable. This will fail to compile. Consider returning a snapshot struct of plain integers (loaded from the atomics), or return a const RefCountStats*/reference with appropriate lifetime/locking semantics.

[Copilot]

Ref<T>::InternalRelease() updates weak refs via ControlBlockRegistry<T>, which is type-specific. If an object is owned by Ref<Derived> but observed via WeakRef<Base> (constructed from Ref<Derived>), the WeakRef<Base> control block lives in ControlBlockRegistry<Base> and will never be invalidated when the last Ref<Derived> deletes the object, leading to WeakRef::Expired() returning false and potential use-after-free on Lock(). The control block registry should be shared across all T (e.g., keyed on RefCounted*/void*), and all Ref/WeakRef operations should use the same registry/control block regardless of static type.

Suggested change

	// Update any weak references before deleting the object
	Internal::ControlBlockRegistry<T>::GetInstance().RemoveControlBlock(m_Ptr);
	// Update any weak references before deleting the object.
	// Use a registry shared across all RefCounted-derived types to avoid
	// type-specific control blocks that can break cross-type weak refs.
	Internal::ControlBlockRegistry<RefCounted>::GetInstance().RemoveControlBlock(m_Ptr);

[Copilot]

The Ref(const std::shared_ptr<...>&) constructors only store shared.get() and then use the internal refcount, but they don't keep the std::shared_ptr control block alive. If the last std::shared_ptr is destroyed while any Ref still exists, the object will be deleted and the Ref will dangle. Either remove these constructors or implement real interop by storing/shared-owning the std::shared_ptr control block (e.g., via an aliasing shared_ptr/bridge) so lifetimes are actually shared.

Suggested change

	* @brief Constructor from std::shared_ptr. Shares ownership of the object.
	*
	* This constructor allows interoperability with std::shared_ptr.
	*
	* @param shared The std::shared_ptr to convert from.
	*/
	explicit Ref(const std::shared_ptr<T>& shared) noexcept : m_Ptr(shared.get())
	{
	InternalAddRef();
	}
	/**
	* @brief Constructor from std::shared_ptr with type conversion.
	*
	* This constructor allows creation of a Ref<T> from a std::shared_ptr<U>
	* where U is convertible to T. This enables interoperability with standard
	* library smart pointers while maintaining type safety.
	*
	* @tparam U Source type that is convertible to T
	* @param shared The std::shared_ptr<U> to convert from
	* @note The object must inherit from RefCounted for proper reference counting
	*/
	template <typename U>
	explicit Ref(const std::shared_ptr<U>& shared) noexcept requires (std::is_convertible_v<U*,T*>) : m_Ptr(static_cast<T*>(shared.get()))
	{
	InternalAddRef();
	}
	* @brief Constructor from std::shared_ptr.
	*
	* This constructor previously attempted interoperability with std::shared_ptr>
	* by storing shared.get() and using the intrusive RefCounted counter. This
	* is unsafe because it does not keep the std::shared_ptr control block alive,
	* which can lead to dangling pointers if the last std::shared_ptr is destroyed
	* while any Ref instances remain.
	*
	* To prevent unsafe lifetime interactions, this constructor is explicitly
	* deleted. Construct Ref<T> using supported constructors instead.
	*
	* @param shared The std::shared_ptr to convert from (unsupported).
	*/
	explicit Ref(const std::shared_ptr<T>& shared) noexcept = delete;
	/**
	* @brief Constructor from std::shared_ptr with type conversion.
	*
	* This constructor previously allowed creation of a Ref<T> from a
	* std::shared_ptr<U> where U is convertible to T. It suffered from the
	* same lifetime issues as the non-converting overload and is therefore
	* explicitly deleted to avoid undefined behavior.
	*
	* @tparam U Source type that is convertible to T
	* @param shared The std::shared_ptr<U> to convert from (unsupported).
	*/
	template <typename U>
	explicit Ref(const std::shared_ptr<U>& shared) noexcept requires (std::is_convertible_v<U*,T*>) = delete;

[Copilot]

Ref::ToSharedPtr() can delete the object via the custom deleter, but this deletion path bypasses Ref<T>::InternalRelease() and therefore does not call ControlBlockRegistry<...>::RemoveControlBlock(). Any existing WeakRef control blocks may keep a non-null pointer, causing Expired()/Lock() to behave as if the object is still alive. Ensure shared_ptr destruction invalidates weak refs too (e.g., route deletion through the same control-block invalidation path or unify control blocks as described earlier).