ActiveObjectMgr fixes

[Desour]

This is a bugfix PR.

These are the ActiveObjectMgr related changes from #13275.

Fixes some iterator invalidation issues, as well as a memleak that you can get by adding an object in on_destruct.

To do

This PR is a Ready for Review.

How to test

• For testing the memleak, you can use this mod:

minetest.register_entity("test_ent_add_on_death:ent1", {
	collisionbox = {-0.5, -0.5, -0.5, 0.5, 0.5, 0.5},
	visual = "cube",
	visual_size = {x = 1, y = 1},
	textures = {},
	static_save = true, -- looks like on_deactivate is not called if this is false

	on_activate = function(self, staticdata, dtime_s)
		minetest.log("[ent1] on_activate")
	end,
	on_deactivate = function(self, removal)
		minetest.log("[ent1] on_deactivate")
		minetest.add_entity(self.object:get_pos(), "test_ent_add_on_death:ent2")
		--~ error("myerror")
	end,
	on_punch = function(self, puncher, time_from_last_punch, tool_capabilities, dir)
		minetest.log("[ent1] on_punch")
		self.object:remove()
	end,
})

minetest.register_entity("test_ent_add_on_death:ent2", {
	collisionbox = {-0.5, -0.5, -0.5, 0.5, 0.5, 0.5},
	visual = "cube",
	visual_size = {x = 2, y = 0.5},
	textures = {},
	static_save = false,

	on_activate = function(self, staticdata, dtime_s)
		minetest.log("[ent2] on_activate")
	end,
	on_deactivate = function(self, removal)
		minetest.log("[ent2] on_deactivate")
	end,
	on_punch = function(self, puncher, time_from_last_punch, tool_capabilities, dir)
		minetest.log("[ent2] on_punch")
		self.object:remove()
	end,
})

And spawn an ent1.
Note that there's a warning at shutdown, and no abort. This is expected.

• Otherwise just make sure objects are doing weird stuff.
• You can also run with sanitizers, if you want. (Hasn't detected any issue with this PR for me.)

[lhofhansl]

I like using unique_ptr. Looks good after a quick glance. I also agree that this is a holy mess!

[JosiahWI]

Should these all be unique_ptrs? It looks like the objects may be a shared resource and should use shared_ptr.

[JosiahWI]

Agree that unique_ptr is good. Nothing to indicate that any of the pointers returned from the object manager are owning.

[Desour]

Should these all be unique_ptrs? It looks like the objects may be a shared resource and should use shared_ptr.

Using weak_ptrs could make sense I guess. But I don't want to do that change here.

Nothing to indicate that any of the pointers returned from the object manager are owning.

They aren't owning. They're owned by the aomgr.

---

Addresses comments and added test instructions. Please review.

[Desour]

Rebased.

[sfan5]

from an efficiency POV this is quite bad
the loop goes from O(n) to O(n * log n) just for lookup (and this runs every step!)

if we set entries to nullptr and postpone deletion wouldn't that solve the problem? IIRC it was just about iterator invalidation

[Desour]

The currently iterated obj isn't necessarily deleted here, and neither in clearIf.

if we set entries to nullptr and postpone deletion

Do you mean in removeObject? That's an interesting idea.
f can also insert new objects. Insertion doesn't invalidate iterators, but it means some of the new objects are immediately iterated through here and others aren't. So I'm not sure about the correctness.

IIRC, these functions are only called once per client or server step. So until this really turns out to be a performance bottleneck, I'd rather opt for the current more naive solution than to attempting premature optimization.

[sfan5]

Do you mean in removeObject? That's an interesting idea.

Yes.

f can also insert new objects. Insertion doesn't invalidate iterators, but it means some of the new objects are immediately iterated through here and others aren't. So I'm not sure about the correctness.

hmmm

IIRC, these functions are only called once per client or server step. So until this really turns out to be a performance bottleneck, I'd rather opt for the current more naive solution than to attempting premature optimization.

IMO we already have enough issues with entity performance and shouldn't be looking to make it worse.

[sfan5]

Idea that's hopefully not too complicated:
A second map is added, let's call it m_new.
If an object is deleted during iteration it is merely set to nullptr (occasional GC is done).
If an object is added during iteration it is added to m_new (getActiveObject will also check here).
After iteration ends m_new is merged into m_active_objects
advantages: No more copying to iterate safely, virtually no extra cost unless the edge cases actually happen.

Edit: We should of course merge these fixes first to get the safety.

[sfan5]

lgtm

[nerzhul]

with many entites you will copy a huge memory block. Use ref as parameter to write result on

[Desour]

Do you mean something like void getAllIds(std::vector<u16> &ret)? That's not more efficient. Function calls are prvalue expressions (see value categories), and there's copy elision. So, there should only be move constructor calls, no copying.

[nerzhul]

aren't we defeating the std::unique_ptr with this method, where we move the unique_ptr then we always have the pointer here and we are playing with it ?

[Desour]

I'm not sure if I understand the question correctly.

obj is a non-owning ref/ptr. object owns. After the registerObject call, someone else owns the object, but references (i.e. obj) are still valid (at least if registerObject returns true).
(Before, there was just the raw ptr object. It was an owning raw ptr when we got it, but after the registerObject call it suddenly was no longer owning.)
The point of unique ptrs is not that you don't use other references on the object anymore.

[nerzhul]

i checked this function signature, i think we can diverge from ServerEnvironment, as there is no inheritance.
this will prevent the just next line getter. Can se return directly the CAO pointer instead (nullptr if register failed) ?

[nerzhul]

by preventing that we will improve performance if there is huge amount of entities, removing one lookup

[Desour]

Sure, that would be possible.
But I'd not like to stuff more changes into this PR.