Skip to content

Fix polygon manipulator aabb computation #903

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

kevyuu
Copy link
Contributor

@kevyuu kevyuu commented Jul 17, 2025

Fix AABB computation on cpu polygon geometry

Comment on lines 74 to 75
const auto& jointWeightView = geo->getJointWeightViews()[vertex_i];
for (auto weight_i = 0u; weight_i < jointWeightView.weights.getElementCount(); weight_i++)

Choose a reason for hiding this comment

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

you're indexing the wrong thing, its not one view per vertex

its supposed to be

// precompute the count outside the lambda to optimize
const auto jointViewCount = geo->isSkinned() ? geo->getJointWeightViews().size():0;

for (auto j=0; j<jointViewCount; j++)
{
   hlsl::float32_t4 weight;
   jointWeightView.weights.decodeElement(vertex_i,weight);
   if (j==jointViewCount-1)
   {
      // can also precompute active channel mask on last view and use `hlsl::any(weight<promote<float32_t4>(0.f) && activeMask)`
      for (auto c=0; c<getChannelCount(jointWeightView.weights.format); c++)
      if (weight[c]>0.f)
          return true;
   }
   else if (hlsl::any(hlsl::promote<hlsl::float32_t4>(0.f)<weight))
      return true;
}
return false;

Choose a reason for hiding this comment

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

if you have Maximum of 1-4 bones affecting a vertex, you only have one joint view, if you have 5-8 you have two joint views (but all except last have formats with 4 components for index and weight, while the last has whatever remains to handle non multiple of 4 max simultaneous bone affections per vertex), etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 192 to 193
static const size_t JOINT_COUNT_PER_VERTEX = 4;

Choose a reason for hiding this comment

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

huh ?

  1. Misleading name, the joints per vertex is an upper bound, also it comes from format channel counts across the joint weight views
  2. should be constexpr static inline otherwise you mess up linking
  3. Wrong naming convention, capital snake case is for macros only, constants are capital starting camel case

Choose a reason for hiding this comment

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

actually just remove it

Choose a reason for hiding this comment

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

still a TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -72,14 +72,20 @@ class NBL_API2 CPolygonGeometryManipulator
{
if (!geo->isSkinned()) return false;
constexpr auto jointCountPerVertex = ICPUPolygonGeometry::SJointWeight::JOINT_COUNT_PER_VERTEX;
for (auto& weightView : geo->getJointWeightViews())
const auto jointViewCount = geo->getJointWeightViews().size();

Choose a reason for hiding this comment

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

compute this at start of function outside the lambda

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 83 to 85
for (auto channel_i = 0; channel_i < getFormatChannelCount(weightView.weights.composed.format); channel_i++)
if (weight[channel_i] > 0.f)
return true;

Choose a reason for hiding this comment

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

just do this for all views, I'm not sure I put a hard requirement that all weight and index views must have 4 component format (I just put a req that component count must match for each weight-index pair)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants