Skip to content

Commit

Permalink
skinning: fix a possible crash/assert
Browse files Browse the repository at this point in the history
all textures declared in a shader must be bound, in the case of the
new skinning API, if less than 4 bone weights are used the texture is 
not needed, but it must still be bound.
  • Loading branch information
pixelflinger committed Sep 24, 2023
1 parent d82b7a5 commit 04c8845
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 23 deletions.
11 changes: 6 additions & 5 deletions filament/src/RenderPass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -875,14 +875,12 @@ void RenderPass::Executor::execute(backend::DriverApi& driver,
info.skinningHandle,
info.skinningOffset * sizeof(PerRenderableBoneUib::BoneData),
sizeof(PerRenderableBoneUib));
// note: always bind the skinningTexture because the shader needs it.
driver.bindSamplers(+SamplerBindingPoints::PER_RENDERABLE_SKINNING,
info.skinningTexture);
// note: even if only skinning is enabled, binding morphTargetBuffer is needed.
driver.bindSamplers(+SamplerBindingPoints::PER_RENDERABLE_MORPHING,
info.morphTargetBuffer);

if (UTILS_UNLIKELY(info.skinningTexture)) {
driver.bindSamplers(+SamplerBindingPoints::PER_RENDERABLE_SKINNING,
info.skinningTexture);
}
}

if (UTILS_UNLIKELY(info.morphWeightBuffer)) {
Expand All @@ -892,6 +890,9 @@ void RenderPass::Executor::execute(backend::DriverApi& driver,
info.morphWeightBuffer);
driver.bindSamplers(+SamplerBindingPoints::PER_RENDERABLE_MORPHING,
info.morphTargetBuffer);
// note: even if only morphing is enabled, binding skinningTexture is needed.
driver.bindSamplers(+SamplerBindingPoints::PER_RENDERABLE_SKINNING,
info.skinningTexture);
}

driver.draw(pipeline, info.primitiveHandle, instanceCount);
Expand Down
29 changes: 13 additions & 16 deletions filament/src/components/RenderableManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -596,19 +596,6 @@ void FRenderableManager::create(
}
}

if (UTILS_UNLIKELY(boneCount > 0) && (builder->mBoneIndicesAndWeightsCount > 0)){
// create and set texture for bone indices and weights
Bones& bones = manager[ci].bones;
FSkinningBuffer::HandleIndicesAndWeights const handle =
downcast(builder->mSkinningBuffer)->createIndicesAndWeightsHandle(
downcast(engine), builder->mBoneIndicesAndWeightsCount);
bones.handleSamplerGroup = handle.sampler;
bones.handleTexture = handle.texture;
downcast(builder->mSkinningBuffer)->
setIndicesAndWeightsData(downcast(engine), handle.texture,
builder->mBoneIndicesAndWeights, builder->mBoneIndicesAndWeightsCount);
}

// Create and initialize all needed MorphTargets.
// It's required to avoid branches in hot loops.
MorphTargets* const morphTargets = new MorphTargets[entryCount];
Expand All @@ -619,10 +606,20 @@ void FRenderableManager::create(

mManager[ci].morphTargets = { morphTargets, size_type(entryCount) };

// Even when morphing isn't enabled, we should create morphing resources.
// Because morphing shader code is generated when skinning is enabled.
// You can see more detail at Variant::SKINNING_OR_MORPHING.
// Always create skinning and morphing resources if one of them is enabled because
// the shader always handles both. See Variant::SKINNING_OR_MORPHING.
if (UTILS_UNLIKELY(boneCount > 0 || targetCount > 0)) {

auto [sampler, texture] = FSkinningBuffer::createIndicesAndWeightsHandle(
downcast(engine), builder->mBoneIndicesAndWeightsCount);
if (builder->mBoneIndicesAndWeightsCount > 0) {
FSkinningBuffer::setIndicesAndWeightsData(downcast(engine), texture,
builder->mBoneIndicesAndWeights, builder->mBoneIndicesAndWeightsCount);
}
Bones& bones = manager[ci].bones;
bones.handleSamplerGroup = sampler;
bones.handleTexture = texture;

// Instead of using a UBO per primitive, we could also have a single UBO for all primitives
// and use bindUniformBufferRange which might be more efficient.
MorphWeights& morphWeights = manager[ci].morphWeights;
Expand Down
5 changes: 3 additions & 2 deletions filament/src/details/SkinningBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,12 @@ void FSkinningBuffer::setBones(FEngine& engine, Handle<backend::HwBufferObject>
constexpr size_t MAX_SKINNING_BUFFER_WIDTH = 2048;

static inline size_t getSkinningBufferWidth(size_t pairCount) noexcept {
return std::min(pairCount, MAX_SKINNING_BUFFER_WIDTH);
return std::clamp(pairCount, size_t(1), MAX_SKINNING_BUFFER_WIDTH);
}

static inline size_t getSkinningBufferHeight(size_t pairCount) noexcept {
return (pairCount + MAX_SKINNING_BUFFER_WIDTH - 1) / MAX_SKINNING_BUFFER_WIDTH;
return std::max(size_t(1),
(pairCount + MAX_SKINNING_BUFFER_WIDTH - 1) / MAX_SKINNING_BUFFER_WIDTH);
}

inline size_t getSkinningBufferSize(size_t pairCount) noexcept {
Expand Down

0 comments on commit 04c8845

Please sign in to comment.