Index count restriction for remap functions

[michael-fadely]

I'm using meshoptimizer to optimize meshes for use on the Sega Dreamcast. Aside from one minor snag, the experience so far has been great! I'm even using it to generate triangle strips with great success.

The first step of my process is remapping to remove unused/duplicate vertices, and this is where I hit that snag. My inputs are stored as indexed triangle or quad lists. This is out of my control as this is for a port of an existing game.

I found that meshopt_generateVertexRemap and related functions assert that the input index buffer length is a multiple of 3, which obviously doesn't work for quads. Definitely correct me if I'm wrong, but that restriction seems unnecessary after skimming through the code. Would it be possible to reconsider this restriction, or is this a deliberate design decision (as obviously the rest of the code genuinely requires it)?

(I convert quads to triangles for simplification and strip generation later in my process, but those steps are optional; sometimes I need quads in, quads out. I can work around the remap restriction by converting or padding my index buffer, but meshoptimizer has spoiled me with convenience, so I figured I'd ask!)

[zeux]

Yeah I've encountered a few cases over the last couple years where the extra restriction seems unnecessary. It's on my list to re-evaluate whether the restrictions in these functions make sense.

In some of these functions at some point in the past, there was a problem where people would accidentally misuse the function and pass wrong inputs; so the assertions are designed to try to protect against it. I would need to look at the history to recall whether it affected specifically generateVertexRemap or any other functions, but I seem to remember that was where the issues originated.

Note that for generateVertexRemap specifically, you could call it on the input vertex data without the indices (indices == NULL, index_count == vertex_count). This disables the %3 check, although the remap should then be used in remapIndexBuffer that has the same assertion unfortunately 😅 The reason I mention that is that remapIndexBuffer is definitely safe to remove the check from, as it's hard to see how that function could be misused - just not sure about the vertex remap.

[zeux]

After looking into this closer I decided it should be safe to remove the restriction from a few functions, notably remap-related and vfetch optimization. For both, the restriction does not affect the implementation and a couple different use cases have come up in the past that make the restriction inconvenient. See #1032.

[michael-fadely]

Thank you for looking into this!