Add custom bounding box properties to model / render components

[vkalpias]

API CHANGE

• aabb property of RenderComponent and ModelComponent has been renamed to customAabb, to allow aabb property to be used for different purpose in the future.
• This also fixes the issue with this customAabb property usage. Before this PR, it was incorrectly handled as if specified in world space. This is correctly being handled as in local space now.

This PR improves / fixes functionality introduced here: #2435

This PR also adds support to specify custom AABB in addComponent for model and render component. This is to allow customAabb to be exposed in the Editor by separate PR.

[Maksims]

Regarding local/world space of AABB: for mesh instance, AABB is in world space, which is handy. I wonder why AABB for model/render component would be in local space, as for culling, this would require extra transform.
Also, it feels strange to provide such properties, as there is already aabb property on the component.
Why not just setting existing aabb, or have a flag that would disable AABB auto-update?

[willeastcott]

Very good point, @Maksims. If you use the aabb setter, this should tell the engine that you are overriding the auto-calculated bounding box, IMHO. I guess my only concern would be around what you'd need to do to switch back from a custom AABB to an auto-calculated one. In that case, maybe a separate boolean property would be the way to go. Or setting aabb to null.

But yeah, the general principle should be to avoid bloating the API if we already have a nice and obvious aabb property.

[mvaligursky]

the aabb property on model and render component is exactly what I added to these classes few months ago when implementing this override system. This PR does not add any new API.

Regarding local vs world space. Mesh provides aabb in local space. If the mesh is skinned or / and morphed, we evaluate local space of that mesh each frame. For culling, this gets then converted to world space, but only in case the transform of the entity has changed.

So this change in the implementation fits in to this. User can provide fixed local space aabb, which is transformed to the world space as needed.

If user would need to specify it in the world space, the user would have to update this (move aabb together with entity) each frame. This way engine simply does it for them in a consistent way. AABB is internally generated in the local space in all cases, and transformed to the world space to be used by the culling.

[mvaligursky]

ah perhaps you meant these two new properties as mentioned in the description:

This PR adds support for 2 properties in addComponent for model and render component:

aabbCenter
aabbHalfExtents

I'm going to reword those, as those are not properties, but only the data the component stores, but internally it's part of existing aabb property of BoundingBox type.

[Maksims]

Regarding local/world, what happens internally - is ok. I'm more concerned about the public API:
meshInstance.aabb - world space AABB, very useful and I've used it often.
model.aabb - named same, but would be local space?
The issue is the hidden distinction between them, but the same variable name. It is not obvious from the API that they behave differently. I would expect to model.aabb - to be world space.
This also is similar to naming of functions: getPosition and getLocalPosition - local is explicit.

What could be done, is providing custom local AABB - if that is present, then AABB will be calculated by the engine using it. If it is not provided, then the engine will auto-calculate.
Also, it would be great to always have model.aabb available, and calculated by the engine automatically or based on provided localAabb.

model.localAabb - null or BoundingBox.

[mvaligursky]

What could be done, is providing custom local AABB - if that is present, then AABB will be calculated by the engine using it. If it is not provided, then the engine will auto-calculate.

This is exactly how it's done.

Regarding the naming .. we use local for position, but not for aabb. Example:

• mesh has aabb property, and that is in the local space
• meshInstance has aabb property, and that is in the world space

So I'd be happy to keep it called aabb.

[Maksims]

• mesh has aabb property, and that is in the local space

Mesh is independent of Scene Graph.

• meshInstance has aabb property, and that is in the world space

Mesh Instance is always dependant on Scene Graph and belongs to an Entity.

ModelComponent/RenderComponent is always dependant on Scene Graph and belongs to an Entity.

That is why within context of Component and Mesh Instance, it is assumed to be world space, not local space.

I propose a small change to this PR:

Instead of two new properties: aabbCenter and aabbHalfExtents to add one property localAabb.
And keep aabb world space, just like for mesh instances. As their main purpose is culling and picking, they are almost always used in world space.

This also makes it easy to work with, as localAabb (BoundingBox) will have all useful functions.

This also will avoid any API breaking changes, only additions.

[mvaligursky]

This PR does not introduce aabbCenter and aabbHalfExtents properties - these are only private storage space in the scene format for the aabb which is an existing property. It's done this way due to Editor not having a native support for BoundingBox in inspector, and so internally it uses two Vec3 members. These are only used to create a component, and never after.

Before this PR, I added aabb property on model and render component recently, and unfortunately an unintended bug crept in and this was stored in the world space. While testing it inside the Editor just now this was discovered, and was corrected to be in local space as intended.

We can rename .aabb to .localAabb (which would be inconsistent with the Mesh.aabb - which is a public interface and is used by the users, so I would prefer to not do it), but we won't be keeping both, at least not part of this PR.

[Maksims]

We can rename .aabb to .localAabb (which would be inconsistent with the Mesh.aabb - which is a public interface and is used by the users, so I would prefer to not do it), but we won't be keeping both, at least not part of this PR.

meshInstance.aabb - is a world space. And in scene graph context (same as render/model component) it makes sense.

Basically, here is a common user story:
Task: need to implement picking of an object.
Implementation (pseudo):

for(meshInstance in modelComponent) {
    if (meshInstance.aabb.intersectsRay(ray)) {
        intersected();
        break;
    }
}

Sometimes it is like so:

for(meshInstance in modelComponent) {
    if (first meshInstance) {
        aabb.copy(meshInstance.aabb);
    } else {
        aabb.add(meshInstance.aabb);
    }
}
if (aabb.intersectsRay(ray)) {
    intersected();
}

If model/render component AABB would be same as mesh instance, in world space, it would be easy to do:

if (entity.render.aabb.intersectsRay(ray)) {
    intersected();
}

But if it is local, then need to do this:

aabb.setFromTransformedAabb(entity.render.aabb, entity.getWorldTransform());
if (aabb.intersectsRay(ray) {
    intersected();
}

Basically, my question: if aabb is used for culling (which is world space), then why model/render component aabb should be local space? What is the motivation behind it?

[mvaligursky]

The goal of this PR (and original PR which I linked in the description) is not to simplify picking of meshes by using bounding boxes. For this, a different API which is picking specific should be designed. For that case though I would probably transform the ray to local space and use local aabb instead of evaluating world space aabb. But that's a separate issue.

This PR is an optimisation for characters (and other meshes that modify vertex positions - either on CPU or in the vertex shader) to allow engine to avoid the expensive evaluation of local aabb for that mesh, by specifying oversized local aabb.

Before, for (even invisible) skinned characters, the engine would need to evaluate all bones, and then evaluate world space aabb of a mesh instance. This was expensive (see stats in the original PR). Now, when oversized local aabb is specified by the user, we don't evaluate bones nor the local space aabb based on those bones, and simply transform local aabb to world space for culling (which is exactly what noon-skinned meshes do). This cuts off lots of expensive computation for off screen characters - nor bones nor bone aabb is evaluated.

For this to work, the oversized aabb needs to be specified on local space .. so that the character can move (by moving entity) and engine can update its world space aabb for culling. If this was specified in world space, a user would additionally need to write a script to update this aabb as character moves.

I'm open to renaming component.aabb to .localAabb, or even overrideAabb, but initial PR introduced .aabb already.

[willeastcott]

OK, sounds reasonable @mvaligursky.

As for API/naming, what about:

• aabb - gettable world space AABB of model or render component [read only]
• customAabb - gettable/settable local space AABB of model or render component

[Maksims]

Great reasoning, I thought that was the case.

This is similar to:

mesh local aabb > mesh instance world aabb

In our case:

component custom/local aabb > component world aabb

So engine can store local aabb or use custom, and re-calculate world aabb when transform or aabb is dirty.

[mvaligursky]

great, I'll rename component.aabb to .customAabb, thanks for the discussion.