Optimize scene node child removal to constant time

[appgurueu]

An attempt at fixing #274, briefly tested with Shadow Forest, but not profiled.

This relies on two invariants in removeChild:

• If a node is a child of another node, its parent property is set appropriately. (And if a node isn't a child of another node, it's parent property must be different from that other node.)
• If a node is a child of another node, the iterator points to the correct list node.

You can experimentally verify that these seem to hold by replacing the body of removeChild with the less efficient:

ISceneNodeList::iterator it = Children.begin();
for (; it != Children.end(); ++it)
	if ((*it) == child)
	{
		assert(child->Parent == this);
		assert(it == child->Iterator);
		child->Iterator = std::nullopt;
		child->Parent = 0;
		child->drop();
		Children.erase(it);
		return true;
	}

assert(child->Parent != this);
return false;

The asserts should not be tripped.

[lhofhansl]

I know we discussed on the issue, but still wanted to ask:

1. Do we need to keep the insert order?
2. If not, can we perf-compare this with just using an unordered_set (assuming children are unique)?

(The idea is clever, btw. Just seems easy to accidentally break in the future.)

[Desour]

Tested, works.

[appgurueu]

Do we need to keep the insert order?

I'm not sure. I can imagine keeping the insertion order to be useful, for example when scene nodes need to be drawn in a particular order due to depth sorting. I don't know whether our current implementation relies on this however, since entities, particles, and mapblocks are all depth-sorted by Minetest.

If not, can we perf-compare this with just using an unordered_set (assuming children are unique)?

We probably could, but I'm not sure an unordered set is a good idea.

Pros:

• Maybe slightly lower constant factors due to reduced allocator overhead.
• Probably slightly easier to maintain long-term.

Cons:

• Should it be deemed a worthwhile optimization, constant factors could also be lowered by giving scene nodes "prev" and "next" pointers, effectively making them list nodes. Since the scene nodes already need to be allocated, this should pretty much get rid of the allocator overhead.
• Larger change.
• Does not preserve order.
• Higher risk of breaking something.
• Unsure about shrinking / rehashing. If you only erase children, they aren't even allowed to do this. So if you go from 1000 children to 10, you will still have memory for 1000 children allocated (unless you manually call rehash, but you'd have to be careful to do that at the right time to not wreck performance).

Just seems easy to accidentally break in the future.

As said on the issue, I believe this is well encapsulated by the add / remove child methods of ISceneNode. If you want further encapsulation, I can stick Children and ChildIterator in their own tiny "class" and make them private, only allowing access through methods which ensure consistency (though I feel that would be overengineered).

I also don't expect this (the children management in particular) to be touched often, looking at the history.

[sfan5]

tested a bit, didn't crash