Follow-up to #3860 - better fix

[Cervator]

#3860 fixes an issue where in some circumstances a NaN value gets mixed into some rendering. At first it seemed to be a sequence issue about a player's respawn location not having been set right before re-entering the player after death, causing a different player to fail trying to draw the player that doesn't have a valid location yet. Something like that, see the other issue for details, near midnight here and running on empty :-)

We're applying a temporary fix to catch and deal with the NaN but that doesn't feel right, like outlined in the PR. Need to revisit - making this issue as a reminder.

[keturn]

There's an additional issue with that patch, beyond the risk of obfuscating the underlying problem.

This added a lot of location.getWorldPosition() calls to a lot of places.

Thinking "it's a Location object, I'll just get one of its coordinates, check it against NaN, fast and easy" is a totally understandable assumption, but it turns out that getWorldPosition().x is not as cheap as just looking up a field on this object you already have a reference to:

Terasology/engine/src/main/java/org/terasology/logic/location/LocationComponent.java

Lines 112 to 117 in d9fdb02

	LocationComponent parentLoc = parent.getComponent(LocationComponent.class);
	while (parentLoc != null) {
	output.scale(parentLoc.scale);
	parentLoc.getLocalRotation().rotate(output, output);
	output.add(parentLoc.position);
	parentLoc = parentLoc.parent.getComponent(LocationComponent.class);

…it checks to see if the entity's parent has a location, adjusts relative to that, and so on up the chain.

It's probably not a long chain. But even for short chains it has very different performance characteristics than the == null checks it was inserted next to.

Methods like ExplosionAuthoritySystem.onActivate will probably never notice the difference. But other methods, like where the player is and what they're looking at get called every tick.

I believe (informed by at least a little empirical data from a quick test run) this has had a significant impact on CPU load.

[skaldarnar]

How/why does that value end up to NaN, and why are not just removing the location component from the entity? What am I missing here?

[Cervator]

@meetcshah19 might remember some more from the original research (there was quite a bit). Will ping on Discord as well :-)

[keturn]

I wasn't sure, so I just did a quick search on whether setting up try/catch adds any overhead for the calls that don't generate exceptions. It looks like there's general agreement that using try/catch does not add overhead.

In that case, given how rarely this happens (measured as percentage-of-method-calls, not percentage-of-multiplayer-games) and that it really is an exceptional case, I think try/catch would serve us much better here than checking up front.

Now exactly where to catch it or what to do about it when it's caught I don't know.

[skaldarnar]

Related to #3984