[Memory leak] Not calling NativeScriptComponent::DestroyScript

[lauralex]

Possible memory leak (?)

Hi, I was analyzing the project for possible bugs and specifically memory leaks. I discovered a possible memory leak in the Components.h file. Is this intended?

[lauralex]

I resolved it like this!....

        struct NativeScriptComponent
	{
		std::shared_ptr<ScriptableEntity> Instance = nullptr;

		std::shared_ptr<ScriptableEntity>(*InstantiateScript)();
		void (*DestroyScript)(NativeScriptComponent*);

		template<typename T>
		void Bind()
		{
			InstantiateScript = []() { return std::dynamic_pointer_cast<ScriptableEntity>(std::make_shared<T>()) ; };
			DestroyScript = [](NativeScriptComponent* nsc) { nsc->Instance.reset(); nsc->Instance = nullptr; };
		}
	};

It runs correctly and without memory leaks now.

[LovelySanta]

This is indeed currently a memory leak, however there is a todo in the Scene class about it: https://github.com/TheCherno/Hazel/blob/c4c66118b26d9a89547fd9230089f6869fc78238/Hazel/src/Hazel/Scene/Scene.cpp#L36 It is also on the Hazel roadmap, it will be resolved as part of the runtime. I'll leave this as a bug, but putting it on hold for now.

[0xworks]

my two cents: I kind of think that "ScriptableEntity" would be better named "NativeScript". A NativeScriptComponent then wraps a NativeScript (similar to TransformComponent wrapping a Transform) Also the thing called ScriptableEntity isn't really an entity, it is a component.

I wonder whether Instance needs to be a shared_ptr. Would unique_ptr be more appropriate? Who other than the parent NativeScriptComponent needs ownership?

nsc->Instance = nullptr; after nsc->reset() is redundant. In fact, if Instance is a smart pointer, then there might not be a need for DestroyScript at all. (or ScriptableEntity::OnDestroy(), as anything you put in there could just as well be put into the ScriptableEntity virtual destructor)

Finally, it would be nice to be able to pass parameters to the instance constructor (including the entity that the script is attached to) Like so:

	struct NativeScriptComponent
	{
		std::function<std::unique_ptr<NativeScript>(Entity entity)> InstantiateScript;
		std::unique_ptr<NativeScript> Instance;

		template<typename T, typename... Args>
		void Bind(Args... args)
		{
			InstantiateScript = [args...](Entity entity) -> std::unique_ptr<NativeScript> { return std::make_unique<T>(entity, args...); };
		}
	};

Then, there is no need for OnCreate(), and instantiation of the script is a bit nicer (plus can pass params through), like so: In the client application:

	auto& nsc = myEntity.AddComponent<Hazel::NativeScriptComponent>();
	nsc.Bind<MySpecialScript>(parameter1, parameter2);

And in Hazel::Scene::OnUpdate() (or wherever it ends up...)

	for(auto&&[entity, nsc] : m_Registry.view<NativeScriptComponent>().each())
	{
		// TODO: Move to Scene::OnScenePlay
		if (!nsc.Instance)
		{
			nsc.Instance = nsc.InstantiateScript({entity, this});    <-- this will set the script to point to the entity, and also pass parameter1 and parameter2 (from the Bind call) to your script components constructor
		}
		nsc.Instance->OnUpdate(ts);
	}
	

[LovelySanta]

Feel free to make a PR to address these (valid) comments