Fix possible change of scale sign when decomposing matrix (#7138)

Co-authored-by: Mathias Agopian <mathias@google.com>

NEW_RELEASE_NOTES.md

@@ -8,4 +8,5 @@ appropriate header in [RELEASE_NOTES.md](./RELEASE_NOTES.md).
 
 ## Release notes for next branch cut
 
+- gltfio: Fix possible change of scale sign when decomposing transform matrix for animation
 - engine: Fixes "stable" shadows (see b/299310624)

android/gltfio-android/CMakeLists.txt

@@ -52,6 +52,7 @@ set(GLTFIO_SRCS
         ${GLTFIO_DIR}/include/gltfio/FilamentInstance.h
         ${GLTFIO_DIR}/include/gltfio/MaterialProvider.h
         ${GLTFIO_DIR}/include/gltfio/NodeManager.h
+        ${GLTFIO_DIR}/include/gltfio/TrsTransformManager.h
         ${GLTFIO_DIR}/include/gltfio/ResourceLoader.h
         ${GLTFIO_DIR}/include/gltfio/TextureProvider.h
         ${GLTFIO_DIR}/include/gltfio/math.h
@@ -69,10 +70,12 @@ set(GLTFIO_SRCS
         ${GLTFIO_DIR}/src/FilamentAsset.cpp
         ${GLTFIO_DIR}/src/FilamentInstance.cpp
         ${GLTFIO_DIR}/src/FNodeManager.h
+        ${GLTFIO_DIR}/src/FTrsTransformManager.h
         ${GLTFIO_DIR}/src/GltfEnums.h
         ${GLTFIO_DIR}/src/Ktx2Provider.cpp
         ${GLTFIO_DIR}/src/MaterialProvider.cpp
         ${GLTFIO_DIR}/src/NodeManager.cpp
+        ${GLTFIO_DIR}/src/TrsTransformManager.cpp
         ${GLTFIO_DIR}/src/ResourceLoader.cpp
         ${GLTFIO_DIR}/src/StbProvider.cpp
         ${GLTFIO_DIR}/src/TangentsJob.cpp

libs/gltfio/CMakeLists.txt

@@ -16,6 +16,7 @@ set(PUBLIC_HDRS
         include/gltfio/FilamentInstance.h
         include/gltfio/MaterialProvider.h
         include/gltfio/NodeManager.h
+        include/gltfio/TrsTransformManager.h
         include/gltfio/ResourceLoader.h
         include/gltfio/TextureProvider.h
         include/gltfio/math.h
@@ -35,10 +36,12 @@ set(SRCS
         src/FilamentAsset.cpp
         src/FilamentInstance.cpp
         src/FNodeManager.h
+        src/FTrsTransformManager.h
         src/GltfEnums.h
         src/Ktx2Provider.cpp
         src/MaterialProvider.cpp
         src/NodeManager.cpp
+        src/TrsTransformManager.cpp
         src/ResourceLoader.cpp
         src/StbProvider.cpp
         src/TangentsJob.cpp

libs/gltfio/include/gltfio/TrsTransformManager.h

@@ -0,0 +1,114 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef GLTFIO_TRSTRANSFORMMANAGER_H
+#define GLTFIO_TRSTRANSFORMMANAGER_H
+
+#include <filament/FilamentAPI.h>
+
+#include <utils/compiler.h>
+#include <utils/EntityInstance.h>
+#include <math/quat.h>
+#include <math/vec3.h>
+#include <math/mat4.h>
+
+using namespace filament::math;
+
+namespace utils {
+class Entity;
+} // namespace utils
+
+namespace filament::gltfio {
+
+class FTrsTransformManager;
+
+/**
+ * TrsTransformManager is used to add entities with glTF-specific trs information.
+ *
+ * Trs information here just used for Animation, DON'T use for transform.
+ */
+class UTILS_PUBLIC TrsTransformManager {
+public:
+    using Instance = utils::EntityInstance<TrsTransformManager>;
+    using Entity = utils::Entity;
+
+    /**
+     * Returns whether a particular Entity is associated with a component of this TrsTransformManager
+     * @param e An Entity.
+     * @return true if this Entity has a component associated with this manager.
+     */
+    bool hasComponent(Entity e) const noexcept;
+
+    /**
+     * Gets an Instance representing the trs transform component associated with the given Entity.
+     * @param e An Entity.
+     * @return An Instance object, which represents the trs transform component associated with the Entity e.
+     * @note Use Instance::isValid() to make sure the component exists.
+     * @see hasComponent()
+     */
+    Instance getInstance(Entity e) const noexcept;
+
+    /**
+     * Creates a trs transform component and associates it with the given entity.
+     * @param entity            An Entity to associate a trs transform component with.
+     * @param translation       The translation to initialize the trs transform component with.
+     * @param rotation          The rotation to initialize the trs transform component with.
+     * @param scale             The scale to initialize the trs transform component with.
+     *
+     * If this component already exists on the given entity, it is first destroyed as if
+     * destroy(Entity e) was called.
+     *
+     * @see destroy()
+     */
+    void create(Entity entity);
+    void create(Entity entity, const float3& translation, const quatf& rotation,
+            const float3& scale);  //!< \overload
+
+    /**
+     * Destroys this component from the given entity.
+     * @param e An entity.
+     *
+     * @see create()
+     */
+    void destroy(Entity e) noexcept;
+
+    void setTranslation(Instance ci, const float3& translation) noexcept;
+    const float3& getTranslation(Instance ci) const noexcept;
+
+    void setRotation(Instance ci, const quatf& rotation) noexcept;
+    const quatf& getRotation(Instance ci) const noexcept;
+
+    void setScale(Instance ci, const float3& scale) noexcept;
+    const float3& getScale(Instance ci) const noexcept;
+
+    void setTrs(Instance ci, const float3& translation, const quatf& rotation,
+            const float3& scale) noexcept;
+    const mat4f getTransform(Instance ci) const noexcept;
+
+protected:
+    TrsTransformManager() noexcept = default;
+    ~TrsTransformManager() = default;
+
+public:
+    TrsTransformManager(TrsTransformManager const&) = delete;
+    TrsTransformManager(TrsTransformManager&&) = delete;
+    TrsTransformManager& operator=(TrsTransformManager const&) = delete;
+    TrsTransformManager& operator=(TrsTransformManager&&) = delete;
+};
+
+} // namespace filament::gltfio
+
+#endif // GLTFIO_TRSTRANSFORMMANAGER_H

libs/gltfio/src/Animator.cpp

@@ -19,6 +19,7 @@
 
 #include "FFilamentAsset.h"
 #include "FFilamentInstance.h"
+#include "FTrsTransformManager.h"
 #include "downcast.h"
 
 #include <filament/VertexBuffer.h>
@@ -74,6 +75,7 @@ struct AnimatorImpl {
     FFilamentInstance* instance = nullptr;
     RenderableManager* renderableManager;
     TransformManager* transformManager;
+    TrsTransformManager* trsTransformManager;
     vector<float> weights;
     FixedCapacityVector<mat4f> crossFade;
     void addChannels(const FixedCapacityVector<Entity>& nodeMap, const cgltf_animation& srcAnim,
@@ -190,6 +192,7 @@ Animator::Animator(FFilamentAsset const* asset, FFilamentInstance* instance) {
     mImpl->instance = instance;
     mImpl->renderableManager = &asset->mEngine->getRenderableManager();
     mImpl->transformManager = &asset->mEngine->getTransformManager();
+    mImpl->trsTransformManager = asset->getTrsTransformManager();
 
     const cgltf_data* srcAsset = asset->mSourceAsset->hierarchy;
     const cgltf_animation* srcAnims = srcAsset->animations;
@@ -429,20 +432,13 @@ void AnimatorImpl::applyAnimation(const Channel& channel, float t, size_t prevIn
         size_t nextIndex) {
     const Sampler* sampler = channel.sourceData;
     const TimeValues& times = sampler->times;
+    TrsTransformManager::Instance trsNode = trsTransformManager->getInstance(channel.targetEntity);
     TransformManager::Instance node = transformManager->getInstance(channel.targetEntity);
 
-    // Perform the interpolation. This is a simple but inefficient implementation; Filament
-    // stores transforms as mat4's but glTF animation is based on TRS (translation rotation
-    // scale).
-    mat4f xform = transformManager->getTransform(node);
-    float3 scale;
-    quatf rotation;
-    float3 translation;
-    decomposeMatrix(xform, &translation, &rotation, &scale);
-
     switch (channel.transformType) {
 
         case Channel::SCALE: {
+            float3 scale;
             const float3* srcVec3 = (const float3*) sampler->values.data();
             if (sampler->interpolation == Sampler::CUBIC) {
                 float3 vert0 = srcVec3[prevIndex * 3 + 1];
@@ -453,10 +449,12 @@ void AnimatorImpl::applyAnimation(const Channel& channel, float t, size_t prevIn
             } else {
                 scale = ((1 - t) * srcVec3[prevIndex]) + (t * srcVec3[nextIndex]);
             }
+            trsTransformManager->setScale(trsNode, scale);
             break;
         }
 
         case Channel::TRANSLATION: {
+            float3 translation;
             const float3* srcVec3 = (const float3*) sampler->values.data();
             if (sampler->interpolation == Sampler::CUBIC) {
                 float3 vert0 = srcVec3[prevIndex * 3 + 1];
@@ -467,10 +465,12 @@ void AnimatorImpl::applyAnimation(const Channel& channel, float t, size_t prevIn
             } else {
                 translation = ((1 - t) * srcVec3[prevIndex]) + (t * srcVec3[nextIndex]);
             }
+            trsTransformManager->setTranslation(trsNode, translation);
             break;
         }
 
         case Channel::ROTATION: {
+            quatf rotation;
             const quatf* srcQuat = (const quatf*) sampler->values.data();
             if (sampler->interpolation == Sampler::CUBIC) {
                 quatf vert0 = srcQuat[prevIndex * 3 + 1];
@@ -481,6 +481,7 @@ void AnimatorImpl::applyAnimation(const Channel& channel, float t, size_t prevIn
             } else {
                 rotation = slerp(srcQuat[prevIndex], srcQuat[nextIndex], t);
             }
+            trsTransformManager->setRotation(trsNode, rotation);
             break;
         }
 
@@ -519,8 +520,7 @@ void AnimatorImpl::applyAnimation(const Channel& channel, float t, size_t prevIn
         }
     }
 
-    xform = composeMatrix(translation, rotation, scale);
-    transformManager->setTransform(node, xform);
+    transformManager->setTransform(node, trsTransformManager->getTransform(trsNode));
 }
 
 void AnimatorImpl::resetBoneMatrices(FFilamentInstance* instance) {

libs/gltfio/src/AssetLoader.cpp

@@ -21,6 +21,7 @@
 
 #include "FFilamentAsset.h"
 #include "FNodeManager.h"
+#include "FTrsTransformManager.h"
 #include "GltfEnums.h"
 
 #include <filament/Box.h>
@@ -295,6 +296,7 @@ public:
     MaterialProvider& mMaterials;
     Engine& mEngine;
     FNodeManager mNodeManager;
+    FTrsTransformManager mTrsTransformManager;
 
     // Transient state used only for the asset currently being loaded:
     FFilamentAsset* mAsset;
@@ -400,7 +402,8 @@ void FAssetLoader::createRootAsset(const cgltf_data* srcAsset) {
     #endif
 
     mDummyBufferObject = nullptr;
-    mAsset = new FFilamentAsset(&mEngine, mNameManager, &mEntityManager, &mNodeManager, srcAsset);
+    mAsset = new FFilamentAsset(&mEngine, mNameManager, &mEntityManager, &mNodeManager,
+            &mTrsTransformManager, srcAsset);
 
     // It is not an error for a glTF file to have zero scenes.
     mAsset->mScenes.clear();
@@ -563,7 +566,9 @@ void FAssetLoader::recurseEntities(const cgltf_data* srcAsset, const cgltf_node*
         quatf* rotation = (quatf*) &node->rotation[0];
         float3* scale = (float3*) &node->scale[0];
         float3* translation = (float3*) &node->translation[0];
-        localTransform = composeMatrix(*translation, *rotation, *scale);
+        mTrsTransformManager.create(entity, *translation, *rotation, *scale);
+        localTransform = mTrsTransformManager.getTransform(
+                mTrsTransformManager.getInstance(entity));
     }
 
     auto parentTransform = mTransformManager.getInstance(parent);

libs/gltfio/src/FFilamentAsset.h

@@ -19,6 +19,7 @@
 
 #include <gltfio/FilamentAsset.h>
 #include <gltfio/NodeManager.h>
+#include <gltfio/TrsTransformManager.h>
 
 #include <filament/Engine.h>
 #include <filament/IndexBuffer.h>
@@ -110,9 +111,9 @@ using MeshCache = utils::FixedCapacityVector<utils::FixedCapacityVector<Primitiv
 struct FFilamentAsset : public FilamentAsset {
     FFilamentAsset(Engine* engine, utils::NameComponentManager* names,
             utils::EntityManager* entityManager, NodeManager* nodeManager,
-            const cgltf_data* srcAsset) :
+            TrsTransformManager* trsTransformManager, const cgltf_data* srcAsset) :
             mEngine(engine), mNameManager(names), mEntityManager(entityManager),
-            mNodeManager(nodeManager),
+            mNodeManager(nodeManager), mTrsTransformManager(trsTransformManager),
             mSourceAsset(new SourceAsset {(cgltf_data*)srcAsset}),
             mTextures(srcAsset->textures_count),
             mMeshCache(srcAsset->meshes_count) {}
@@ -195,6 +196,10 @@ struct FFilamentAsset : public FilamentAsset {
         return mEngine;
     }
 
+    TrsTransformManager* getTrsTransformManager() const noexcept {
+        return mTrsTransformManager;
+    }
+
     void releaseSourceData() noexcept;
 
     const void* getSourceAsset() const noexcept {
@@ -242,6 +247,7 @@ struct FFilamentAsset : public FilamentAsset {
     utils::NameComponentManager* const mNameManager;
     utils::EntityManager* const mEntityManager;
     NodeManager* const mNodeManager;
+    TrsTransformManager* const mTrsTransformManager;
     std::vector<utils::Entity> mEntities; // sorted such that renderables come first
     std::vector<utils::Entity> mLightEntities;
     std::vector<utils::Entity> mCameraEntities;

libs/gltfio/src/FTrsTransformManager.h

@@ -0,0 +1,165 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef GLTFIO_FTRSTRANSFORMMANAGER_H
+#define GLTFIO_FTRSTRANSFORMMANAGER_H
+
+#include "downcast.h"
+#include "gltfio/math.h"
+#include "math/quat.h"
+#include "utils/debug.h"
+
+#include <gltfio/TrsTransformManager.h>
+
+#include <utils/compiler.h>
+#include <utils/SingleInstanceComponentManager.h>
+#include <utils/Entity.h>
+#include <utils/FixedCapacityVector.h>
+#include <utils/Slice.h>
+
+namespace filament::gltfio {
+
+class UTILS_PRIVATE FTrsTransformManager : public TrsTransformManager {
+public:
+    using Instance = TrsTransformManager::Instance;
+
+    FTrsTransformManager() noexcept {}
+
+    ~FTrsTransformManager() noexcept {
+        assert_invariant(mManager.getComponentCount() == 0);
+    }
+
+    void terminate() noexcept;
+
+    bool hasComponent(utils::Entity e) const noexcept {
+        return mManager.hasComponent(e);
+    }
+
+    Instance getInstance(utils::Entity e) const noexcept {
+        return Instance(mManager.getInstance(e));
+    }
+
+    void create(utils::Entity entity) {
+        create(entity, float3{}, quatf{}, float3{1});
+    }
+
+    void create(utils::Entity entity, const float3& translation,
+                const quatf& rotation, const float3& scale) {
+        if (UTILS_UNLIKELY(mManager.hasComponent(entity))) {
+            destroy(entity);
+        }
+        UTILS_UNUSED_IN_RELEASE Instance ci = mManager.addComponent(entity);
+        assert_invariant(ci);
+
+        if (ci) {
+            setTrs(ci, translation, rotation, scale);
+        }
+    }
+
+    void destroy(utils::Entity e) noexcept {
+        if (Instance ci = mManager.getInstance(e); ci) {
+            mManager.removeComponent(e);
+        }
+    }
+
+    void gc(utils::EntityManager& em) noexcept {
+        mManager.gc(em);
+    }
+
+    void setTranslation(Instance ci, const float3& translation) noexcept {
+        assert_invariant(ci.isValid());
+        mManager[ci].translation = translation;
+    }
+
+    const float3& getTranslation(Instance ci) const noexcept {
+        return mManager[ci].translation;
+    }
+
+    void setRotation(Instance ci, const quatf& rotation) noexcept {
+        assert_invariant(ci.isValid());
+        mManager[ci].rotation = rotation;
+    }
+
+    const quatf& getRotation(Instance ci) const noexcept {
+        return mManager[ci].rotation;
+    }
+
+    void setScale(Instance ci, const float3& scale) noexcept {
+        assert_invariant(ci.isValid());
+        mManager[ci].scale = scale;
+    }
+
+    const float3& getScale(Instance ci) const noexcept {
+        return mManager[ci].scale;
+    }
+
+    void setTrs(Instance ci, const float3& translation,
+            const quatf& rotation, const float3& scale) noexcept {
+        setTranslation(ci, translation);
+        setRotation(ci, rotation);
+        setScale(ci, scale);
+    }
+
+    const mat4f getTransform(Instance ci) const noexcept {
+        return composeMatrix(getTranslation(ci), getRotation(ci), getScale(ci));
+    }
+
+private:
+    enum {
+        TRANSLATION,
+        ROTATION,
+        SCALE,
+    };
+
+    using Base = utils::SingleInstanceComponentManager<
+            float3,
+            quatf,
+            float3>;
+
+    struct Sim : public Base {
+        using Base::gc;
+        using Base::swap;
+
+        typename Base::SoA& getSoA() { return mData; }
+
+        struct Proxy {
+            UTILS_ALWAYS_INLINE
+            Proxy(Base& sim, utils::EntityInstanceBase::Type i) noexcept :
+                    translation{ sim, i } { }
+
+            union {
+                Field<TRANSLATION>   translation;
+                Field<ROTATION>      rotation;
+                Field<SCALE>         scale;
+            };
+        };
+
+        UTILS_ALWAYS_INLINE Proxy operator[](Instance i) noexcept {
+            return { *this, i };
+        }
+        UTILS_ALWAYS_INLINE const Proxy operator[](Instance i) const noexcept {
+            return { const_cast<Sim&>(*this), i };
+        }
+    };
+
+    Sim mManager;
+};
+
+FILAMENT_DOWNCAST(TrsTransformManager)
+
+} // namespace filament::gltfio
+
+#endif // GLTFIO_FTRSTRANSFORMMANAGER_H

libs/gltfio/src/FilamentAsset.cpp

@@ -60,6 +60,11 @@ FFilamentAsset::~FFilamentAsset() {
 
     }
 
+    // Destroy gltfio trs transform components.
+    for (auto entity : mEntities) {
+        mTrsTransformManager->destroy(entity);
+    }
+
     // Destroy all renderable, light, transform, and camera components,
     // then destroy the actual entities. This includes instances.
     if (!mDetachedFilamentComponents) {

libs/gltfio/src/TrsTransformManager.cpp

@@ -0,0 +1,98 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "FTrsTransformManager.h"
+
+#include <utils/Log.h>
+
+#include "downcast.h"
+#include "gltfio/TrsTransformManager.h"
+
+using namespace utils;
+
+namespace filament::gltfio {
+
+using Instance = TrsTransformManager::Instance;
+
+void FTrsTransformManager::terminate() noexcept {
+    auto& manager = mManager;
+    if (!manager.empty()) {
+#ifndef NDEBUG
+        utils::slog.d << "cleaning up " << manager.getComponentCount()
+            << " leaked trs transform components" << utils::io::endl;
+#endif
+        while (!manager.empty()) {
+            Instance ci = manager.end() - 1;
+            manager.removeComponent(manager.getEntity(ci));
+        }
+    }
+}
+
+bool TrsTransformManager::hasComponent(Entity e) const noexcept {
+    return downcast(this)->hasComponent(e);
+}
+
+Instance TrsTransformManager::getInstance(Entity e) const noexcept {
+    return downcast(this)->getInstance(e);
+}
+
+void TrsTransformManager::create(Entity entity) {
+    downcast(this)->create(entity);
+}
+
+void TrsTransformManager::create(Entity entity, const float3& translation,
+        const quatf& rotation, const float3& scale) {
+    downcast(this)->create(entity, translation, rotation, scale);
+}
+
+void TrsTransformManager::destroy(Entity e) noexcept {
+    downcast(this)->destroy(e);
+}
+
+void TrsTransformManager::setTranslation(Instance ci, const float3& translation) noexcept {
+    downcast(this)->setTranslation(ci, translation);
+}
+
+const float3& TrsTransformManager::getTranslation(Instance ci) const noexcept {
+    return downcast(this)->getTranslation(ci);
+}
+
+void TrsTransformManager::setRotation(Instance ci, const quatf& rotation) noexcept {
+    downcast(this)->setRotation(ci, rotation);
+}
+
+const quatf& TrsTransformManager::getRotation(Instance ci) const noexcept {
+    return downcast(this)->getRotation(ci);
+}
+
+void TrsTransformManager::setScale(Instance ci, const float3& scale) noexcept {
+    downcast(this)->setScale(ci, scale);
+}
+
+const float3& TrsTransformManager::getScale(Instance ci) const noexcept {
+    return downcast(this)->getScale(ci);
+}
+
+void TrsTransformManager::setTrs(Instance ci, const float3& translation,
+        const quatf& rotation, const float3& scale) noexcept {
+    downcast(this)->setTrs(ci, translation, rotation, scale);
+}
+
+const mat4f TrsTransformManager::getTransform(Instance ci) const noexcept {
+    return downcast(this)->getTransform(ci);
+}
+
+} // namespace filament::gltfio