From 00f2659a8691c9c5c74d63d8403302cfa8c0c9cd Mon Sep 17 00:00:00 2001 From: Jaremie Romer <33360084+jaremieromer@users.noreply.github.com> Date: Thu, 4 Jan 2024 13:06:15 -0600 Subject: [PATCH] Code review feedback --- .../graphics/SkeletalAnimationTypes.h | 11 +- .../system/SkeletalAnimationCalculations.cpp | 118 ++++++++++-------- .../system/SkeletalAnimationSystem.cpp | 36 +++--- .../system/SkeletalAnimationTypes.cpp | 8 +- 4 files changed, 100 insertions(+), 73 deletions(-) diff --git a/include/ncengine/graphics/SkeletalAnimationTypes.h b/include/ncengine/graphics/SkeletalAnimationTypes.h index a8471b4eb..a7b4589e1 100644 --- a/include/ncengine/graphics/SkeletalAnimationTypes.h +++ b/include/ncengine/graphics/SkeletalAnimationTypes.h @@ -57,6 +57,15 @@ struct State std::string animUid; }; +/** + * @brief Used to traverse the flattened offsets tree. + */ +struct OffsetChildren +{ + uint32_t indexOfFirstChild; + uint32_t numChildren; +}; + /** * @brief An SoA representation of nc::asset::BonesData. * @@ -70,7 +79,7 @@ struct PackedRig std::vector boneToParent; DirectX::XMMATRIX globalInverseTransform; std::vector boneNames; - std::vector> boneToParentIndices; + std::vector offsetChildren; std::vector offsetsMap; }; diff --git a/source/engine/graphics/system/SkeletalAnimationCalculations.cpp b/source/engine/graphics/system/SkeletalAnimationCalculations.cpp index 3689587f8..89d25da3f 100644 --- a/source/engine/graphics/system/SkeletalAnimationCalculations.cpp +++ b/source/engine/graphics/system/SkeletalAnimationCalculations.cpp @@ -32,12 +32,12 @@ auto GetAnimationOffsets(float timeInTicks, continue; } - animationMatrices.offsets.emplace_back(anim::DecomposedMatrix - { - .pos = GetInterpolatedPosition(timeInTicks, iter->second.positionFrames), - .rot = GetInterpolatedRotation(timeInTicks, iter->second.rotationFrames), - .scale = GetInterpolatedScale(timeInTicks, iter->second.scaleFrames) - }); + animationMatrices.offsets.emplace_back + ( + GetInterpolatedPosition(timeInTicks, iter->second.positionFrames), + GetInterpolatedRotation(timeInTicks, iter->second.rotationFrames), + GetInterpolatedScale(timeInTicks, iter->second.scaleFrames) + ); animationMatrices.hasValues.push_back(1); } return animationMatrices; @@ -47,14 +47,11 @@ auto ComposeMatrices(float timeInTicks, const std::vector& boneNames, const nc::asset::SkeletalAnimation* animation) -> anim::PackedAnimation { - auto decomposedAnimation = GetAnimationOffsets(timeInTicks, boneNames, animation); - - auto packedAnimation = anim::PackedAnimation{}; - auto elementsCount = decomposedAnimation.offsets.size(); - packedAnimation.offsets.reserve(elementsCount); - packedAnimation.hasValues = std::vector{decomposedAnimation.hasValues.begin(), decomposedAnimation.hasValues.end()}; + auto [offsets, hasValues] = GetAnimationOffsets(timeInTicks, boneNames, animation); + auto packedAnimation = anim::PackedAnimation{ .offsets = std::vector{}, .hasValues = std::move(hasValues) }; + packedAnimation.offsets.reserve(packedAnimation.hasValues.size()); - std::ranges::transform(decomposedAnimation.offsets, std::back_inserter(packedAnimation.offsets), [](auto&& offset) + std::ranges::transform(offsets, std::back_inserter(packedAnimation.offsets), [](auto&& offset) { return ToScaleMatrix(offset.scale) * ToRotMatrix(offset.rot) * ToTransMatrix(offset.pos); }); @@ -90,26 +87,31 @@ auto ComposeBlendedMatrices(float blendFromTime, auto AnimateBones(const anim::PackedRig& rig, const anim::PackedAnimation& anim) -> std::vector { - // Copy the boneToParent vector to perform modifications in place. - auto boneToParentSandbox = std::vector{rig.boneToParent.begin(), rig.boneToParent.end()}; - auto boneToParentSandboxSize = boneToParentSandbox.size(); - // Replace each boneToParent offset with its animation offset, if present. Else, leave as the original offset. - for (auto i = 0u; i < boneToParentSandboxSize; i++) - { - boneToParentSandbox[i] = (boneToParentSandbox[i] * !anim.hasValues[i]) + (anim.offsets[i] * anim.hasValues[i]); - } + auto boneToParent = std::vector{}; + boneToParent.reserve(rig.boneToParent.size()); + std::ranges::transform( + std::views::zip(rig.boneToParent, anim.offsets, anim.hasValues), + std::back_inserter(boneToParent), + [](auto&& in) + { + auto&& [parent, offset, hasValue] = in; + return parent * !hasValue + offset * hasValue; + } + ); // Multiply each child (siblings are contiguous) with its parent. - for (auto i = 0u; i < boneToParentSandboxSize; i++) - { - auto [parentFirstChild, parentChildren] = rig.boneToParentIndices[i]; - - for (auto& j : std::views::counted(boneToParentSandbox.begin() + parentFirstChild, parentChildren)) // Update the child to be itself times its parent offset above. This incorporates recursive parent/animation data into the offsets vector. + std::ranges::for_each( + std::views::zip(rig.offsetChildren, boneToParent), + [&boneToParent](auto&& in) { - j = j * boneToParentSandbox[i]; + auto&& [children, parent] = in; + std::ranges::for_each( + std::views::counted(boneToParent.begin() + children.indexOfFirstChild, children.numChildren), + [&parent](auto& child){ parent *= child; } + ); } - } + ); // Create the output vector. auto animatedBones = std::vector{}; @@ -117,12 +119,15 @@ auto AnimateBones(const anim::PackedRig& rig, // Create a final transform for each bone by multiplying the (vertex-space-to-bone-space matrix) with the (bone-space-to-animated-parent-bone-space matrix) with the (global inverse transform matrix). // This outputs a matrix that can be used to transform a vertex into its final animated position. - auto globalInverseTransform = rig.globalInverseTransform; - auto vertexToBoneSize = rig.vertexToBone.size(); - for (auto i = 0u; i < vertexToBoneSize; i++) - { - animatedBones.push_back(nc::graphics::SkeletalAnimationData{rig.vertexToBone[i] * boneToParentSandbox[rig.offsetsMap[i]] * globalInverseTransform}); - } + std::ranges::transform( + std::views::zip(rig.vertexToBone, rig.offsetsMap), + std::back_inserter(animatedBones), + [globalInverseTransform = rig.globalInverseTransform, &boneToParent](auto&& in) + { + auto&& [matrix, offset] = in; + return SkeletalAnimationData{matrix * boneToParent.at(offset) * globalInverseTransform}; + } + ); return animatedBones; } @@ -131,15 +136,18 @@ auto GetInterpolatedPosition(float timeInTicks, const std::vector 0, "Animation has no position data for the node."); if (positionFrames.size() == 1) return positionFrames[0].position; - auto nextFrame = std::find_if(positionFrames.begin(), positionFrames.end(), [timeInTicks](auto&& frame){ return timeInTicks < frame.timeInTicks;}); - auto frame = std::prev(nextFrame); - NC_ASSERT(nextFrame != positionFrames.end() && nextFrame != positionFrames.begin(), fmt::format("Animation has no position data at time {}", timeInTicks)); + const auto nextFrame = std::ranges::find_if( + std::views::drop(positionFrames, 1), + [timeInTicks](auto&& frame){ return timeInTicks < frame.timeInTicks;} + ); - auto deltaTimeInTicks = nextFrame->timeInTicks - frame->timeInTicks; - auto interpolationFactor = (timeInTicks - frame->timeInTicks) / deltaTimeInTicks; + NC_ASSERT(nextFrame != positionFrames.end() && nextFrame != positionFrames.begin(), fmt::format("Animation has no position data at time {}", timeInTicks)); + const auto frame = std::prev(nextFrame); + const auto deltaTimeInTicks = nextFrame->timeInTicks - frame->timeInTicks; + const auto interpolationFactor = (timeInTicks - frame->timeInTicks) / deltaTimeInTicks; NC_ASSERT(interpolationFactor >= 0.0f && interpolationFactor <= 1.0f, fmt::format("Error calculating the interpolation factor: {}", interpolationFactor)); - return Lerp(frame->position, nextFrame->position, static_cast(interpolationFactor)); + return Lerp(frame->position, nextFrame->position, interpolationFactor); } auto GetInterpolatedRotation(float timeInTicks, const std::vector& rotationFrames) -> nc::Quaternion @@ -147,15 +155,18 @@ auto GetInterpolatedRotation(float timeInTicks, const std::vector 0, "Animation has no rotation data for the node."); if (rotationFrames.size() == 1) return Normalize(rotationFrames[0].rotation); - auto nextFrame = std::find_if(rotationFrames.begin(), rotationFrames.end(), [timeInTicks](auto&& frame){ return timeInTicks < frame.timeInTicks; }); - auto frame = std::prev(nextFrame); - NC_ASSERT(nextFrame != rotationFrames.end() && nextFrame != rotationFrames.begin(), fmt::format("Animation has no rotation data at time {}", timeInTicks)); + const auto nextFrame = std::ranges::find_if( + std::views::drop(rotationFrames, 1), + [timeInTicks](auto&& frame){ return timeInTicks < frame.timeInTicks;} + ); - auto deltaTimeInTicks = nextFrame->timeInTicks - frame->timeInTicks; - auto interpolationFactor = (timeInTicks - frame->timeInTicks) / deltaTimeInTicks; + NC_ASSERT(nextFrame != rotationFrames.end() && nextFrame != rotationFrames.begin(), fmt::format("Animation has no rotation data at time {}", timeInTicks)); + const auto frame = std::prev(nextFrame); + const auto deltaTimeInTicks = nextFrame->timeInTicks - frame->timeInTicks; + const auto interpolationFactor = (timeInTicks - frame->timeInTicks) / deltaTimeInTicks; NC_ASSERT(interpolationFactor >= 0.0f && interpolationFactor <= 1.0f, fmt::format("Error calculating the interpolation factor: {}", interpolationFactor)); - return Normalize(Slerp(frame->rotation, nextFrame->rotation, static_cast(interpolationFactor))); + return Normalize(Slerp(frame->rotation, nextFrame->rotation, interpolationFactor)); } auto GetInterpolatedScale(float timeInTicks, const std::vector& scaleFrames) -> nc::Vector3 @@ -163,14 +174,17 @@ auto GetInterpolatedScale(float timeInTicks, const std::vector 0, "Animation has no scale data for the node."); if (scaleFrames.size() == 1) return scaleFrames[0].scale; - auto nextFrame = std::find_if(scaleFrames.begin(), scaleFrames.end(), [timeInTicks](auto&& frame){ return timeInTicks < frame.timeInTicks; }); - auto frame = std::prev(nextFrame); - NC_ASSERT(nextFrame != scaleFrames.end() && nextFrame != scaleFrames.begin(), fmt::format("Animation has no scale data at time {}", timeInTicks)); + const auto nextFrame = std::ranges::find_if( + std::views::drop(scaleFrames, 1), + [timeInTicks](auto&& frame){ return timeInTicks < frame.timeInTicks;} + ); - auto deltaTimeInTicks = nextFrame->timeInTicks - frame->timeInTicks; - auto interpolationFactor = (timeInTicks - frame->timeInTicks) / deltaTimeInTicks; + NC_ASSERT(nextFrame != scaleFrames.end() && nextFrame != scaleFrames.begin(), fmt::format("Animation has no scale data at time {}", timeInTicks)); + const auto frame = std::prev(nextFrame); + const auto deltaTimeInTicks = nextFrame->timeInTicks - frame->timeInTicks; + const auto interpolationFactor = (timeInTicks - frame->timeInTicks) / deltaTimeInTicks; NC_ASSERT(interpolationFactor >= 0.0f && interpolationFactor <= 1.0f, fmt::format("Error calculating the interpolation factor: {}", interpolationFactor)); - return Lerp(frame->scale, nextFrame->scale, static_cast(interpolationFactor)); + return Lerp(frame->scale, nextFrame->scale, interpolationFactor); } } // namespace nc::graphics diff --git a/source/engine/graphics/system/SkeletalAnimationSystem.cpp b/source/engine/graphics/system/SkeletalAnimationSystem.cpp index 2fd6e1c14..979891995 100644 --- a/source/engine/graphics/system/SkeletalAnimationSystem.cpp +++ b/source/engine/graphics/system/SkeletalAnimationSystem.cpp @@ -22,8 +22,13 @@ auto PrepareAnimation(nc::graphics::anim::UnitOfWork& unit, const nc::graphics:: return nc::graphics::ComposeBlendedMatrices(blendFromTimeTicks, blendToTimeTicks, unit.blendFactor, rig.boneNames, unit.blendFromAnim, unit.blendToAnim); } - float* timeInSeconds = unit.blendFromAnim ? &unit.blendFromTime : &unit.blendToTime; - nc::asset::SkeletalAnimation* animData = unit.blendFromAnim ? unit.blendFromAnim : unit.blendToAnim; + auto [timeInSeconds, animData] = [&unit]() + { + return unit.blendFromAnim ? + std::pair{&unit.blendFromTime, unit.blendFromAnim} : + std::pair{&unit.blendToTime, unit.blendToAnim}; + }(); + *timeInSeconds = fmod((*timeInSeconds + dt), (static_cast(animData->durationInTicks)/animData->ticksPerSecond)); auto timeInTicks = *timeInSeconds * animData->ticksPerSecond; return nc::graphics::ComposeMatrices(timeInTicks, rig.boneNames, animData); @@ -72,14 +77,13 @@ auto SkeletalAnimationSystem::Execute() -> SkeletalAnimationSystemState auto state = SkeletalAnimationSystemState{}; const auto dt = time::DeltaTime(); - for (auto i = 0u; i < m_units.size(); i++) + for (auto&& [unit, unitEntity] : std::views::zip(m_units, m_unitEntities)) { - auto& unit = m_units.at(i); auto& rig = *unit.rig; - auto unitIndex = m_unitEntities[i].Index(); + const auto unitIndex = unitEntity.Index(); if (unit.blendFactor < 1.0f) unit.blendFactor += dt * 2.0f; - if (HasCompletedAnimationCycle(unit, dt)) m_registry->Get(m_unitEntities[i])->CompleteFirstRun(); + if (HasCompletedAnimationCycle(unit, dt)) m_registry->Get(unitEntity)->CompleteFirstRun(); auto packedAnimation = PrepareAnimation(unit, rig, dt); auto animatedBones = AnimateBones(rig, packedAnimation); @@ -160,16 +164,16 @@ void SkeletalAnimationSystem::Start(const anim::PlayState& playState) if (pos == m_unitEntities.end()) { m_unitEntities.emplace_back(playState.entity); - m_units.emplace_back(anim::UnitOfWork - { - .index = playState.entity.Index(), - .rig = &m_rigs.at(playState.meshUid), - .blendFromAnim = playState.prevAnimUid.empty() ? nullptr : &m_animationAssets.at(playState.prevAnimUid), - .blendToAnim = playState.curAnimUid.empty() ? nullptr : &m_animationAssets.at(playState.curAnimUid), - .blendFromTime = 0.0f, - .blendToTime = 0.0f, - .blendFactor = 0.0f - }); + m_units.emplace_back + ( + playState.entity.Index(), + &m_rigs.at(playState.meshUid), + playState.prevAnimUid.empty() ? nullptr : &m_animationAssets.at(playState.prevAnimUid), + playState.curAnimUid.empty() ? nullptr : &m_animationAssets.at(playState.curAnimUid), + 0.0f, + 0.0f, + 0.0f + ); return; } diff --git a/source/engine/graphics/system/SkeletalAnimationTypes.cpp b/source/engine/graphics/system/SkeletalAnimationTypes.cpp index 9792e5171..bbcaedcda 100644 --- a/source/engine/graphics/system/SkeletalAnimationTypes.cpp +++ b/source/engine/graphics/system/SkeletalAnimationTypes.cpp @@ -23,7 +23,7 @@ PackedRig::PackedRig(const nc::asset::BonesData& bonesData) boneToParent{}, globalInverseTransform{GetGlobalInverseTransform(bonesData.boneSpaceToParentSpace)}, boneNames{}, - boneToParentIndices{}, + offsetChildren{}, offsetsMap{} { auto& bspsVec = bonesData.boneSpaceToParentSpace; @@ -32,7 +32,7 @@ PackedRig::PackedRig(const nc::asset::BonesData& bonesData) vertexToBone.reserve(vsbsVec.size()); boneToParent.reserve(bspsVec.size()); boneNames.reserve(bspsVec.size()); - boneToParentIndices.reserve(bspsVec.size()); + offsetChildren.reserve(bspsVec.size()); offsetsMap.reserve(vsbsVec.size()); std::ranges::transform(vsbsVec, std::back_inserter(vertexToBone), @@ -44,8 +44,8 @@ PackedRig::PackedRig(const nc::asset::BonesData& bonesData) std::ranges::transform(bspsVec, std::back_inserter(boneNames), [](const nc::asset::BoneSpaceToParentSpace& bsps) -> std::string { return bsps.boneName; }); - std::ranges::transform(bspsVec, std::back_inserter(boneToParentIndices), - [](const nc::asset::BoneSpaceToParentSpace& bsps) -> std::pair { return std::make_pair(bsps.indexOfFirstChild, bsps.numChildren); }); + std::ranges::transform(bspsVec, std::back_inserter(offsetChildren), + [](const nc::asset::BoneSpaceToParentSpace& bsps) -> OffsetChildren { return OffsetChildren { .indexOfFirstChild = bsps.indexOfFirstChild, .numChildren = bsps.numChildren }; }); // Create a vector of uint32_t that represent the vertexOffset's corresponding bone index in the boneSpace offset vector. // This is important because we need to keep the two vectors in sync but the boneSpace vector contains data not present or need in the vertexOffset vector.