Skip to content
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.

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

Trove: The behavior of AttachToInstance differs from what is documented #147

Open
lolmanurfunny opened this issue Aug 4, 2023 · 5 comments

Comments

@lolmanurfunny
Copy link

The :AttachToInstance() method listens to Instance.Destroying, this is no doubt unintended as the docs imply that it uses Instance.AncestryChanged.

The doc's description: "... Once this instance is removed from the game (parent or ancestor's parent set to nil), the trove will automatically clean up."

However, due to the nature of the Instance.Destroying event, the trove will not clean up unless the instance's :Destroy() method was explicitly called.


https://github.com/Sleitnick/RbxUtil/blob/main/modules/trove/init.lua#L370

@ghost
Copy link

ghost commented Aug 14, 2023

However, due to the nature of the Instance.Destroying event, the trove will not clean up unless the instance's :Destroy() method was explicitly called.

This is not entirely true, the event can be internally fired too if the instance is destroyed through other means by the engine (such as an unanchored part falling off the World where position.Y >= Workspace.FallenPartsDestroyHeight.

@lolmanurfunny
Copy link
Author

However, due to the nature of the Instance.Destroying event, the trove will not clean up unless the instance's :Destroy() method was explicitly called.

This is not entirely true, the event can be internally fired too if the instance is destroyed through other means by the engine (such as an unanchored part falling off the World where position.Y >= Workspace.FallenPartsDestroyHeight.

Are you sure this is the case? I attempted to reproduce this behavior as you described, in Studio. It appears that you are in fact completely wrong, that's putting it lightly.

38f8ca055a963127dbf818bb6ad9d2b5.mp4

@ghost
Copy link

ghost commented Aug 15, 2023

image

@lolmanurfunny https://devforum.roblox.com/t/instancedestroy-only-replicates-parent-nil-change/127327/14

Sure, that example I presented may not be the right one since as it seems, the engine does not seem to destroy parts that fall off the world map (this is a sign of an internal memory leak!), but rather parents them to nil:

local p = Instance.new("Part", workspace)

p.Destroying:Connect(function()
	print("ee") -- never prints
end)

p.AncestryChanged:Connect(function(_, parent)
	print(parent) -- nil
end)

p.Position = Vector3.new(0,workspace.FallenPartsDestroyHeight - 10,0)

task.wait(2)

p.Parent = workspace -- Works fine too (If it was destroyed, this would've resulted in an error as the parent would've been locked

My point still stands fully - the internal engine code can call Destroy on instances too and thus result in the Destroying event being fired off.

@Sleitnick
Copy link
Owner

Dealing with Roblox instances can be frustrating. If the ancestor of an instance is set to nil, it can be hard to know if this was done by the engine or by a script. I think the underlying code here should probably be switched back to AncestryChanged (which is what it used to be).

To be clear, I think AttachToInstance probably shouldn't exist on Trove. It inverts the ownership. Ideally, code that wants to clean up a trove based on an instance should write code to bind it to the instance themselves, thus giving the developer the ability to choose between ancestry change or destroy.

@lolmanurfunny
Copy link
Author

To be clear, I think AttachToInstance probably shouldn't exist on Trove. It inverts the ownership. Ideally, code that wants to clean up a trove based on an instance should write code to bind it to the instance themselves, thus giving the developer the ability to choose between ancestry change or destroy.

I agree. I've never needed to utilize AttachToInstance myself since IMO it typically makes more sense to use a kind of lifecycle (eg: Death) to cleanup troves. IME, instance deletion is the effect of trove cleanup more often than it is the cause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants