From 4c55fc3c480ad9eceedd2058738113105612db4a Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 6 Dec 2022 11:05:40 -0800 Subject: [PATCH] fix TransformManager high precision mode The residual would be corrupted when using transactions. --- RELEASE_NOTES.md | 2 + filament/src/components/TransformManager.cpp | 61 +++++++++++--------- filament/src/components/TransformManager.h | 4 +- filament/test/filament_test.cpp | 16 +++-- 4 files changed, 48 insertions(+), 35 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 51f50b3146..ff5fc78be1 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -12,9 +12,11 @@ A new header is inserted each time a *tag* is created. - engine: add missing getters on `MaterialInstance` - WebGL: add missing `ColorGrading` JS bindings - engine: improvements/cleanup of Shadow mapping code [⚠️ **Recompile Materials**] +- engine: fix TransformManager high precision mode when using transactions. ## v1.28.3 + - backend: add support for GGP platform - engine: primitives with `CullingMode::FRONT_AND_BACK` are now skipped. diff --git a/filament/src/components/TransformManager.cpp b/filament/src/components/TransformManager.cpp index 81c64f4210..9324d5ac6e 100644 --- a/filament/src/components/TransformManager.cpp +++ b/filament/src/components/TransformManager.cpp @@ -56,7 +56,7 @@ void FTransformManager::create(Entity entity, Instance parent, const mat4f& loca if (UTILS_UNLIKELY(manager.hasComponent(entity))) { destroy(entity); } - Instance i = manager.addComponent(entity); + Instance const i = manager.addComponent(entity); assert_invariant(i); assert_invariant(i != parent); @@ -78,7 +78,7 @@ void FTransformManager::create(Entity entity, Instance parent, const mat4& local if (UTILS_UNLIKELY(manager.hasComponent(entity))) { destroy(entity); } - Instance i = manager.addComponent(entity); + Instance const i = manager.addComponent(entity); assert_invariant(i); assert_invariant(i != parent); @@ -96,7 +96,7 @@ void FTransformManager::setParent(Instance i, Instance parent) noexcept { validateNode(i); if (i) { auto& manager = mManager; - Instance oldParent = manager[i].parent; + Instance const oldParent = manager[i].parent; if (oldParent != parent) { // TODO: on debug builds, ensure that the new parent isn't one of our descendant removeNode(i); @@ -144,7 +144,7 @@ TransformManager::children_iterator FTransformManager::getChildrenEnd(Instance) void FTransformManager::destroy(Entity e) noexcept { // update the reference of the element we're removing auto& manager = mManager; - Instance i = manager.getInstance(e); + Instance const i = manager.getInstance(e); validateNode(i); if (i) { // 1) remove the entry from the linked lists @@ -158,7 +158,7 @@ void FTransformManager::destroy(Entity e) noexcept { } // 2) remove the component - Instance moved = manager.removeComponent(e); + Instance const moved = manager.removeComponent(e); // 3) update the references to the entry now with Instance i if (moved != i) { @@ -200,14 +200,15 @@ void FTransformManager::updateNodeTransform(Instance i) noexcept { // find our parent's world transform, if any // note: by using the raw_array() we don't need to check that parent is valid. - Instance parent = manager[i].parent; - computeWorldTransform(manager[i].world, manager[i].worldTranslationLo, + Instance const parent = manager[i].parent; + FTransformManager::computeWorldTransform( + manager[i].world, manager[i].worldTranslationLo, manager[parent].world, manager[i].local, manager[parent].worldTranslationLo, manager[i].localTranslationLo, mAccurateTranslations); // update our children's world transforms - Instance child = manager[i].firstChild; + Instance const child = manager[i].firstChild; if (UTILS_UNLIKELY(child)) { // assume we don't have a hierarchy in the common case transformChildren(manager, child); } @@ -237,10 +238,11 @@ void FTransformManager::computeAllWorldTransforms() noexcept { while (UTILS_UNLIKELY(Instance(manager[i].parent) > i)) { swapNode(i, manager[i].parent); } - Instance parent = manager[i].parent; + Instance const parent = manager[i].parent; assert_invariant(parent < i); - computeWorldTransform(manager[i].world, manager[i].worldTranslationLo, + FTransformManager::computeWorldTransform( + manager[i].world, manager[i].worldTranslationLo, manager[parent].world, manager[i].local, manager[parent].worldTranslationLo, manager[i].localTranslationLo, accurate); @@ -258,7 +260,7 @@ void FTransformManager::insertNode(Instance i, Instance parent) noexcept { manager[i].next = 0; if (parent) { // we insert ourselves first in the parent's list - Instance next = manager[parent].firstChild; + Instance const next = manager[parent].firstChild; manager[i].next = next; // we're our parent's first child now manager[parent].firstChild = i; @@ -279,8 +281,10 @@ void FTransformManager::swapNode(Instance i, Instance j) noexcept { auto& manager = mManager; // swap the content of the nodes directly - std::swap(manager.elementAt(i), manager.elementAt(j)); - std::swap(manager.elementAt(i), manager.elementAt(j)); + std::swap(manager.elementAt(i), manager.elementAt(j)); + std::swap(manager.elementAt(i), manager.elementAt(j)); + std::swap(manager.elementAt(i), manager.elementAt(j)); + std::swap(manager.elementAt(i), manager.elementAt(j)); manager.swap(i, j); // this swaps the data relative to SingleInstanceComponentManager // now swap the linked-list references, to do that correctly we must use a temporary @@ -310,13 +314,13 @@ void FTransformManager::swapNode(Instance i, Instance j) noexcept { updateNode(j); } -// removes an node from the graph, but doesn't removes it or its children from the array +// removes a node from the graph, but doesn't remove it or its children from the array // (making everybody orphaned). void FTransformManager::removeNode(Instance i) noexcept { auto& manager = mManager; - Instance parent = manager[i].parent; - Instance prev = manager[i].prev; - Instance next = manager[i].next; + Instance const parent = manager[i].parent; + Instance const prev = manager[i].prev; + Instance const next = manager[i].next; if (prev) { manager[prev].next = next; } else if (parent) { @@ -341,9 +345,9 @@ void FTransformManager::removeNode(Instance i) noexcept { void FTransformManager::updateNode(Instance i) noexcept { auto& manager = mManager; // update our preview sibling's next reference (to ourselves) - Instance parent = manager[i].parent; - Instance prev = manager[i].prev; - Instance next = manager[i].next; + Instance const parent = manager[i].parent; + Instance const prev = manager[i].prev; + Instance const next = manager[i].next; if (prev) { manager[prev].next = i; } else if (parent) { @@ -371,14 +375,15 @@ void FTransformManager::transformChildren(Sim& manager, Instance i) noexcept { const bool accurate = mAccurateTranslations; while (i) { // update child's world transform - Instance parent = manager[i].parent; - computeWorldTransform(manager[i].world, manager[i].worldTranslationLo, + Instance const parent = manager[i].parent; + FTransformManager::computeWorldTransform( + manager[i].world, manager[i].worldTranslationLo, manager[parent].world, manager[i].local, manager[parent].worldTranslationLo, manager[i].localTranslationLo, accurate); // assume we don't have a deep hierarchy - Instance child = manager[i].firstChild; + Instance const child = manager[i].firstChild; if (UTILS_UNLIKELY(child)) { transformChildren(manager, child); } @@ -423,14 +428,14 @@ void FTransformManager::computeWorldTransform( } -void FTransformManager::validateNode(Instance i) noexcept { +void FTransformManager::validateNode(UTILS_UNUSED_IN_RELEASE Instance i) noexcept { #ifndef NDEBUG auto& manager = mManager; if (i) { - Instance parent = manager[i].parent; - Instance firstChild = manager[i].firstChild; - Instance prev = manager[i].prev; - Instance next = manager[i].next; + Instance const parent = manager[i].parent; + Instance const firstChild = manager[i].firstChild; + Instance const prev = manager[i].prev; + Instance const next = manager[i].next; assert_invariant(parent != i); assert_invariant(prev != i); assert_invariant(next != i); diff --git a/filament/src/components/TransformManager.h b/filament/src/components/TransformManager.h index 83d6a9971d..e7a674c8b0 100644 --- a/filament/src/components/TransformManager.h +++ b/filament/src/components/TransformManager.h @@ -130,7 +130,7 @@ private: void computeAllWorldTransforms() noexcept; - void computeWorldTransform(math::mat4f& outWorld, math::float3& inoutWorldTranslationLo, + static void computeWorldTransform(math::mat4f& outWorld, math::float3& inoutWorldTranslationLo, math::mat4f const& pt, math::mat4f const& local, math::float3 const& ptTranslationLo, math::float3 const& localTranslationLo, bool accurate); @@ -166,7 +166,7 @@ private: typename Base::SoA& getSoA() { return mData; } struct Proxy { - // all of this gets inlined + // all of these gets inlined UTILS_ALWAYS_INLINE Proxy(Base& sim, utils::EntityInstanceBase::Type i) noexcept : local{ sim, i } { } diff --git a/filament/test/filament_test.cpp b/filament/test/filament_test.cpp index d7326cbebe..790be957a6 100644 --- a/filament/test/filament_test.cpp +++ b/filament/test/filament_test.cpp @@ -181,6 +181,7 @@ TEST(FilamentTest, SkinningMath) { TEST(FilamentTest, TransformManager) { filament::FTransformManager tcm; + tcm.setAccurateTranslationsEnabled(true); EntityManager& em = EntityManager::get(); std::array entities; em.create(entities.size(), entities.data()); @@ -245,8 +246,9 @@ TEST(FilamentTest, TransformManager) { EXPECT_LT(tcm.getInstance(entities[1]), tcm.getInstance(entities[2])); // local transaction reorders parent/child + auto const t = mat4::translation(double3(1.0 / 3.0)); tcm.openLocalTransformTransaction(); - tcm.setTransform(newParent, mat4f{ float4{ 8 }}); + tcm.setTransform(newParent, t); tcm.commitLocalTransformTransaction(); // local transaction invalidates Instances @@ -258,10 +260,14 @@ TEST(FilamentTest, TransformManager) { EXPECT_GT(child, newParent); // check transform propagation - EXPECT_EQ(tcm.getTransform(newParent), mat4f{ float4{ 8 }}); - EXPECT_EQ(tcm.getWorldTransform(newParent), mat4f{ float4{ 8 }}); - EXPECT_EQ(tcm.getTransform(child), mat4f{ float4{ 1 }}); - EXPECT_EQ(tcm.getWorldTransform(child), mat4f{ float4{ 8 }}); + + // our "high precision" mode only preserves 48 bits of the double mantissa (out of 53). + // This constant loses 5 bits out of 1/3 + const mat4 PRECISION_KILLER_5BITS = mat4::translation(double3(16.0)); + EXPECT_EQ(tcm.getTransformAccurate(newParent) + PRECISION_KILLER_5BITS, t + PRECISION_KILLER_5BITS); + EXPECT_EQ(tcm.getWorldTransformAccurate(newParent) + PRECISION_KILLER_5BITS, t + PRECISION_KILLER_5BITS); + EXPECT_EQ(tcm.getWorldTransformAccurate(child) + PRECISION_KILLER_5BITS, t + PRECISION_KILLER_5BITS); + EXPECT_EQ(tcm.getTransformAccurate(child), mat4{ 1.0 }); // check children iterators size_t c = 0;