SimplifyModifier: cache vertex indices to speed faces

[NNskelly]

Related issue: none

Description

Tell vertices what their index is while we're already touching all of them (O(n)), rather than searching for them 3 times per face (O(m*n)). I concede that bolting extra data onto an established object mid-run is architecturally dubious, but the time saved is on the order of minutes for a 300k vertex mesh.
Aside: I'm also doing some other work which is showing a further ~25% time save by keeping vertices sorted by edge cost (and in the process codifying Vertex.index as a member at construction time) rather than searching for lowest every collapse iteration, but the logical change is considerably deeper, so I'm testing the waters with this hopefully shoo-in change to be sure I have the process right before proposing anything larger.

[mrdoob]

Adding properties to classes tends to have performance penalties.

However, I see that the Vertex class has an id and seems to be unused?

three.js/examples/js/modifiers/SimplifyModifier.js

Line 445 in 9a2fc99

this.id = id; // old index id

Maybe we can just rename it to index?

[Mugen87]

BTW: Please modify examples/jsm/modifiers/SimplifyModifier.js . Files in examples/js are getting generated.

[NNskelly]

pushed the change to jsm. I was actually wondering about the id field; found it when I was formally adding index as a more actively managed field. It's commented as being the original mesh vertex id, which is different from the order in vertices after a bunch are removed, and very different from the order in vertices in my ongoing work after vertices is sorted by edge cost. If the collective experts are willing to sign off that nobody is ever going to want to link back to the original mesh, I could easily repurpose that member. That said, the performance penalties introduced by making the engine track a dynamically added member are empirically far less than the performance hit of running linear-time searches.

[NNskelly]

actually, why even debate this? Changed 'index' to 'newId' for consistency and added it to the constructor. That should satisfy performance concerns, and if whoever is responsible for the old id field wants it gone, that can be decoupled from this optimization.

[Mugen87]

I suggest to use id as mentioned earlier and not introduce a new field. If id is currently not used in the modifier's code, I think it's better to give it a new purpose.

[NNskelly]

If you're confident id is unused, then sure. That said, if this change is taking use/ownership of id , I see no reason to pass it in the constructor when it's not needed until later on and on fewer instances. (init to -1 should be benign for now, and dovetail with the sorting work if that goes through later).

[mrdoob]

Thanks!