BufferGeometry: toIndexed

Since i already posted the half snippet, this patches THREE.BufferGeometry with a function to create a indexed one from a non-indexed. A new BufferGeometry is returned.

A indexed geometry reduces the data of all attributes and will generally perform better, also computeVertexNormals can be called only on indexed geometries to compute smooth normals.

7 Likes

I’ve made an update to this, all attributes are now considered, if vertices don’t share all attributes they remain individual. Also skinning and morph attributes.

I use it in a converter process for my assets since some modelling tools or exporters will generate non-indexed geometries once they use disconnected UV nets, what is mostly the case for sliced up UVs.

The old simple approach is still available since it doesn’t add much code, if you call geometry.toIndexed( false ); it will only merge by position. Reason can be a raw model, or if you want to compute vertex normals regardless of disconnected UVs.

Let me know if you face any issues, since it should be fine with any scenario now i’d make a pull request if all cases are covered.

4 Likes

That is a nice feature. (I haven’t inspected the implementation (yet).)

The reason for that, I figure, is that it requries first indexing faces by which vertices they are connected to. This is much easier to do, and can be done unambiguously, with indexed geometries. To achieve the same result with unindexed geometries, one will in practice have to do about all that is needed to convert to an indexed geometry.

You are using spatial hashing with configurable precision, which should be OK. :+1:
The meaning of the precision parameter is basically the number of digits that must match. OK

Line 35-92 can be greatly simplified by employing a nested loop.

The else clauses in line 256-277 can be omitted.

Each vertex needs all normals of the faces it’s connected to to compute a weighted normal. So if there are UV seams it ignores it’s neighbours which would cause a visible seam later with lights. In some cases this is a desired effect like at steep angles.

It’s for performance reasons, since there are only 4 cases that’s fine.

A bounding sphere is required for frustum culling.

Did you test? I would expect that a modern JS engine would be happy to get a short and intuitive nested for loop expression to work with for optimization. But on the other hand, you tell it beforehand which few values itemSize can be, and that means it can possibly be optimized statically.

A bounding sphere is required for frustum culling.

Yes, but it will be generated on demand by the renderer. It makes sense to copy the source sphere to spare an extra computeBoundingSphere, but there should be no rush with making a new one if none was present. That only adds code lines. (If I am not mistaken.)

You shouldn’t hope it optimizes it somehow for you, anything with clear cases you can tell the compiler in advance, such as initialization values avoids a lot steps.

There shouldn’t be even the case none is present, optimally the bounding sphere and box should be delivered with the asset, it isn’t really for free computing those.

This isn’t really optimal being JIT for any more advanced use cases. As mentioned optimally it would already come with the asset, i even precompute one per hierarchy level for better culling on object level, any system where it’s needed like collissions or directly in game mechanics you would have to check and perform this first.

Thank you for your insight. Would it also help the engine to do this?

if (! ([1,2,3,4].includes(itemSize) ) {
    console.error("Invalid itemSize %d.", itemSize);
    return null;
}

Now the subsequent nested loop can in principle be unrolled on the valid itemSize cases statically.

OK. I agree that it is wise to deliver them with the asset. (I am not used to thinking in assets, as I like to generate geometry on the fly.) But what if your user is not done moving the vertices? Then the automatic computeBoundingSphere is just an annoying extra wait. To me it looks a bit like you are “forcing” your way onto the user, since the else clauses are for specifically those cases where no bounding sphere is present yet, which may have been a user choice, (e.g. because that happens later in their workflow.) But it’s no big deal, of course. Easy to change if any of the conservative voices among the three.js maintainers asks.

I hope your PR gets merged, as this would be a nice addition to three.js. :raised_hands:

@Fyrestar have you made a PR for this?

I hope your PR gets merged, as this would be a nice addition to three.js. :raised_hands:

@Fyrestar have you made a PR for this?

Not to take take away from @Fyrestar’s efforts but there’s already a mergeVertices function in BufferGeometryUtils that I added earlier this year which should handle all these cases, as well. I’m sure there’s something to be gained by sharing code between the implementations, though!

1 Like