Skip to content

Conversation

@show50726
Copy link
Contributor

BUGS=[443321184]

@show50726 show50726 added the internal Issue/PR does not affect clients label Dec 23, 2025
Comment on lines 58 to 59
bool hasPositions() const noexcept;
bool hasTangents() const noexcept;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two could be inline too.

engine.getZeroTexture(), {});

if (UTILS_UNLIKELY(skinning.handle || morphing.handle)) {
if (UTILS_UNLIKELY(skinning.handle)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change makes me nervous. before if you had only morphing enabled, the skinning descriptors were set, but not anymore. I think this part doesn't need to change at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you're right. To make the behavior consistent, I reverted this part of changes.

@show50726
Copy link
Contributor Author

I don't like that this structure becomes 3 bytes.

I think what would make sense is to do a pre-change to this one, where this structure is split in two Visibility and Skinning for instance. The new Skinning structure would contain the data for skinning and morphing. (2 bits initially, 4 bits after your change).

I think this sounds reasonable. Let me create a PR to separate the struct first and will address these comments later on.

@show50726 show50726 force-pushed the dev/custom-morphing branch from ac85cd4 to f2c2cda Compare January 7, 2026 07:37
@show50726 show50726 requested a review from pixelflinger January 7, 2026 07:46
FILAMENT_CHECK_PRECONDITION(mPbHandle)
<< "setPositionsAt() called on a MorphTargetBuffer without a position buffer. Use "
"withPositions(true) in the Builder.";
if (!mPbHandle) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you'd need this if you already have the precondition.

FILAMENT_CHECK_PRECONDITION(mPbHandle)
<< "setPositionsAt() called on a MorphTargetBuffer without a position buffer. Use "
"withPositions(true) in the Builder.";
if (!mPbHandle) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

FILAMENT_CHECK_PRECONDITION(mTbHandle)
<< "setTangentsAt() called on a MorphTargetBuffer without a tangent buffer. Use "
"withTangents(true) in the Builder.";
if (!mTbHandle) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

bool skinning, bool morphing, bool contactShadows, bool hasInstanceBuffer,
bool skinning, uint8_t morphing, bool contactShadows, bool hasInstanceBuffer,
uint8_t channels) noexcept {
uint32_t morphingFlags = uint32_t(morphing) << 9;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Issue/PR does not affect clients

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants