Don't automatically compute Aabb components for skinned meshes.

[pcwalton]

Currently, Bevy automatically computes AABBs for all meshes, even those that have skins or morph targets. This is incorrect, as each skin and/or morph target can deform the mesh arbitrarily. This is not a theoretical problem, as Maya relies on rest poses to position and rotate meshes that are children of skins.

Right now, the only way to disable this Bevy feature is to add the NoFrustumCulling component, or to overwrite the generated Aabb with a different one. Neither are a particularly good experience:

1. Adding NoFrustumCulling is impossible when loading a glTF scene, and it's also not obvious that NoFrustumCulling is needed to correctly render skinned meshes sometimes.

2. Overwriting the Aabb is cumbersome.

This commit changes Bevy's behavior to not generate Aabb components for skinned meshes, fixing the issue. Note that, to keep this patch small, morph targets aren't handled yet. The documentation has been updated to inform developers that they may wish to add Aabb components manually to skinned meshes in order to opt in to frustum culling.

Fixes #4971.

[janhohenheim]

Some open questions:

• What are the performance implications of this for the average user, now that their animated meshes suddenly no longer get frustum culled?
• What do other engines do? I think you said Unity does this approach here. What about Unreal and Godot?
• Would it make sense to include some kind of utility to calculate the max bounds of the AABBs of an animation so users don't need to go manually check their DCC?
• Why is this better than upstreaming https://github.com/greeble-dev/bevy_mod_skinned_aabb? I assume performance? If so, does the performance argument also hold for the average user that "just wants to play an animation" and has no idea they're "supposed" to add an AABB manually?

I'm not against this PR, don't get me wrong. Our default behavior is blatantly wrong and needs to go. I remember how confused I was when I first ran into disappearing meshes due to it.
Just have some reservations about the average end-user experience before I click accept :)

Also requesting reviews from Greeble, as they wrote the crate linked above, and James, who I remember wanted to change the skinned mesh AABB behavior at some point too and has some thoughts on what to replace it with

[pcwalton]

Ok, so looking at what Unity does more closely, it seems that Unity can do something similar to bevy_mod_skinned_aabb but it’s off by default “for performance reasons”. By default, what Unity does is to compute the maximum AABB given all the animations in the FBX and statically caches that, with the ability to override manually if needed. We could do this with glTF by going through the animations, but that wouldn’t help me, as we store various animations for skinned meshes outside the glTF that defines the mesh.

My inclination is:

• What Bevy currently does is just broken and we need to change something. It is never correct to use the rest pose of a skinned mesh for culling. Almost anything would be better than that.
• I’m hesitant to enable bevy_mod_skinned_aabb by default, as I think most games want to use static AABBs defined by artists and don’t want to pay that CPU cost.
• Perhaps we should do what Unity does and compute AABBs based on the maximum bounds of all animations attached to a mesh in a glTF.
• We should have a mode on the glTF loader that disables computing AABBs, even if we do what Unity does and compute a maximal AABB based on all the animations attached to the skin, because of situations like mine in which we can play animations on our skinned meshes that come from outside the glTF that defines the mesh.

I don’t have time to implement the Unity thing, but I can scale this patch back to be an opt in “don’t compute AABBs for skinned meshes” option on the glTF loader if we feel that’d be the right thing.

[janhohenheim]

Good assessment. I'm not sure if it's better to

• change the default
• have an opt-in
• remove the old behavior entirely.

I'll leave that decision to the maintainers. PR looks good other than that decision :)

[tychedelia]

I'm agnostic about the follow up but this is fine as unbreaking the status quo.

[pcwalton]

From a glance, Godot seems to do what bevy_mod_skinned_aabb does. I was unable to determine what Unreal does from a quick search.

I'm persuaded that this patch should be pared down to be an opt-in (or opt-out?) option on the glTF loader that disables automatic AABB generation for skinned meshes and inserts NoFrustumCulling on those meshes. This would handle the vast majority of cases, I think, as I believe that inserting SkinnedMesh components manually (as opposed to loading them from a glTF file) is rare. As follow-up patches, we can then (a) create AABBs that are the union of all animations and (b) upstream bevy_mod_skinned_aabb. Once those patches land, we'll effectively have three modes: (1) manual artist-defined AABBs (what I'll use); (2) automatic tight AABB generation at the cost of CPU time; (3) automatic approximate AABB generation on import (only correct if all the animations that will ever be played on the skin are present in the glTF and if the bone transforms are never changed manually).

This patch should wait to land until I can make that change.

[NicoZweifel]

I checked the UE source and it uses a PhysicsAsset.

If no PhysicsAsset is attached or the opt-out is enabled (bComponentUseFixedSkelBounds) the aabb is static and has the same frustum culling issues as described above, there are a bunch of other conditions (e.g. uniform scaling) but the default from what I understand boils down to a physics based proxy (i.e. ragdoll) and a dynamic aabb that gets calculated every frame.

[greeble-dev]

Gonna add a few notes although they're kinda tangential to this PR. I think that this PR is the practical choice right now - either the current version or the pared down alternative.

I was unable to determine what Unreal does from a quick search.

(EDIT: Oops, I didn't see NicoZweifel's comment before posting this, but I'll leave my notes for reference)

Unreal defaults to creating collision shapes for joints during mesh import. These are used to calculate the culling bounds, even if they're not instantiated in the physics system. So it's like bevy_mod_skinned_aabb, but inefficient and fragile.

Perhaps we should do what Unity does and compute AABBs based on the maximum bounds of all animations attached to a mesh in a glTF.

My hot take on this is that the juice isn't worth the squeeze - as you mentioned there's a bunch of reasons it's awkward and unreliable.

Side note in case it's useful: for most humanoid character rigs - i.e. rigs where joints below the pelvis usually only rotate - there's a simple approximation. For each leaf joint, sum its length and the lengths of all its ancestors, then take the maximum of this value across all leaf joints, and attach an AABB/sphere of that size to the pelvis. In practice this is a bit bigger than an AABB calculated from animations, but way simpler.

As follow-up patches, we can ... upstream bevy_mod_skinned_aabb.

I would be mildly negative on upstreaming bevy_mod_skinned_aabb in its current form. It's a bit too hacky and unreliable (e.g. breaks if meshes are not RenderAssetUsages::MAIN_WORLD), and less efficient than it could be. But I would be open to upstreaming if a maintainer says they understand the limitations, and it's viewed as an experimental option that's only a first step towards a better solution.

I would also like to do an in-engine version that's reliable and more performant. But it's kinda awkward to do right now. The main issues are:

1. The ideal place to calculate the bounds is in extract_skins. But that's after the CPU culling path has run, and I'm reluctant to make it dependent on GPU culling.
2. There are other options, like adding it to the CPU culling. But they cost performance/reliability and would be more controversial.

This situation could change. Maybe the render world gets removed, maybe the culling system gets redesigned, maybe WebGL is dropped and GPU culling is the default. And after the dust has settled my job could be easier. But for now I'm waiting to see what happens.

I can expand on these reasons if any render folks want to chew it over.

I’m hesitant to enable bevy_mod_skinned_aabb by default, as I think most games want to use static AABBs defined by artists and don’t want to pay that CPU cost.

I understand the hesitation, but I would advocate for it to be the default - so static AABBs would be an optional optimisation for sophisticated users. I think per-joint bounds are cheaper than many people expect, and the value of making (non-morph) skinned meshes reliable is very high. There's also some hybrid approaches that trade off reliability for performance, like approximating faces and hands with a single bound.

If upstreaming moves forward then I'll provide more details and concrete performance numbers.

[janhohenheim]

@greeble-dev regarding upstreaming: certainly these limitations are better than the status quo? Especially since e.g. not using the right world simply defaults to not generating an AABB, which is not worse than what we have now.
Right now from a user perspective, some objects just magically disappear sometimes with no indication of why, it's not even clear that animations are to blame. Hard to be more brittle than that.

[greeble-dev]

@greeble-dev regarding upstreaming: certainly these limitations are better than the status quo?

I would agree, but I think there's a decent chance that maintainers might disagree, or the PR would stall over performance doubts. Particularly if this PR lands and things (kinda sorta) work by default. So for now the cost/benefit isn't there for me. But I can change my mind if there's an indication that maintainers will be ok with an unreliable/inefficient version.

[alice-i-cecile]

I'm fine WRT almost any solution here, and would prefer to merge a fix. Off-screen performance issues are less bad than meshes suddenly vanishing.

I understand the hesitation, but I would advocate for it to be the default - so static AABBs would be an optional optimisation for sophisticated users.

This is my preferred long-term solution, but I won't block on it in this PR and would prefer to land a simple fix first.

[greeble-dev]

So @janhohenheim's comment kinda guilted me into taking another look at upstreaming options for bevy_mod_skinned_aabb, and I think I have a compromise with a reasonable chance of landing. It's a bit simpler and more reliable, but less accurate. I expect to file a PR later this week - sneak preview here: 11b3a8d.

I don't think my PR will necessarily conflict with this PR or other alternatives. I would advocate for my PR to be the default behaviour, but that's not a requirement, and either way the alternatives should still be available as an option.

[pcwalton]

@greeble-dev I'm quite happy to close this in favor of your solution, and review your solution, if you submit the PR. Honestly, I just want some solution here--I have no strong opinions as to what in particular we do.

What I'm working on is a band-aid solution that simply takes the rest pose into account when computing the AABB at import time and nothing else. This is still wrong, but better than what we do, and should be uncontroversial as more or less a strict improvement on what we currently do. However, if you're going to submit a better solution I'm happy to hold off.

[janhohenheim]

@greeble-dev oh no, I certainly didn't want to guilt you ❤️

[alice-i-cecile]

@pcwalton, please bother me when you're happy with a bandaid solution :) I'll be happy to take a final look and likely merge it in.