[Srinidhi444]

Resolves #9313

Changes:

This is a Draft PR for the Geometry / GeometryPart refactor discussed with @davepagurek. I'll be implementing and committing the changes incrementally.

Completed:

• Made Geometry extend GeometryPart.
• Removed duplicated constructor initialization while preserving existing behavior.
• Added regression tests for inheritance, constructor callbacks, shared single-part state, and independent geometry instances.

Planned:

• Consolidate shared geometry methods into GeometryPart.
• Move renderer vertex-count gating into the individual parts loop.
• Improve edge and stroke handling for multipart geometry.
• Add relevant regression tests and verify existing rendering behavior.

This cleanup should reduce duplicated geometry logic and provide a cleaner foundation for the UV-related fixes discussed in #9304.

Screenshots of the change:

Not applicable — internal architecture refactor.

PR Checklist

• npm run lint passes
• [Inline reference] is included / updated
• [Unit tests] are included / updated

Tests passed so far:

• npx vitest run test/unit/webgl/p5.GeometryPart.js
• npx vitest run test/unit/webgl/p5.Geometry.js

[Srinidhi444]

Hey @davepagurek! Just wanted to share a quick update on the Geometry / GeometryPart cleanup I've been working on today.

So far, I've:

• Made Geometry extend GeometryPart and removed the duplicated constructor state while preserving existing single-part behavior and constructor callbacks.
• Consolidated vertexProperty() and hasFillTransparency() into GeometryPart. The latter also removes the old transparency cache, which could become stale when vertex colors changed.
• Moved _getFaceNormal() and computeTangents() into GeometryPart, since they operate on local mesh data. This also makes tangent computation available to individual material parts.
• Added regression tests for inheritance and the shared functionality.

The relevant geometry tests have been passing.
Next I'll tackle the renderer's top-level vertex-count gating, followed by multipart edge/stroke handling.

[davepagurek]

Thanks for the updates @Srinidhi444, looking good so far!

One other thing I thought of that I wanted to put on your radar is buildGeometry, since I think that will be affected by this (maybe just the edge handling? not 100% sure yet, I haven't looked at the file again.)

[davepagurek]

We probably don't need to do this kind of test for every property since ideally just extends GeometryPart does all that. Doesn't hurt to have but in terms of a focus for future tests, it's maybe more productive focusing on behaviour that uses those methods rather than just existence

[Srinidhi444]

Ya that makes sense was just adding them to test locally will remove the existing tests like these in future commits.

[Srinidhi444]

Thanks for pointing out buildGeometry(), @davepagurek!

I've completed the renderer gating changes as well. Previously, model() and _drawGeometry() depended on the aggregate geometry's vertex count, which could prevent valid parts from rendering when the parent had no vertices. I've updated the checks to allow rendering based on individual parts and added regression tests. All the relevant test suites are passing.

Regarding buildGeometry(), I investigated it and found that GeometryBuilder.addGeometry() currently copies edges into the aggregate Geometry, offsetting their indices into the combined vertex array:

if (this.renderer.states.strokeColor) {
  for (const edge of input.edges.map(edge =>
    edge.map(idx => idx + startIdx)
  )) {
    this.geometry.edges.push(edge);
  }
}

Meanwhile, _addToCurrentPart() copies fill-related attributes into individual GeometryPart objects but doesn't copy edges.

The stroke pipeline also operates on the aggregate geometry through _edgesToVertices() and _drawStrokes(geometry), so moving edges directly into individual parts could affect stroke joins, caps, and material boundaries.

For now, I'm thinking of preserving the aggregate stroke representation while consolidating the shared geometry functionality.

I'd appreciate your feedback on this approach, and please let me know if I've missed anything or misunderstood part of the implementation!

[davepagurek]

Probably we should be consistent in what gets added where. Ideally everything keeps getting added to the same part, I think, until it hits something that is not able to be in the same part, like a changed texture or material setting. Currently though we don't actually record textures, I think? So possibly we could decide to keep this as-is for now but do everything into a single part, and in the future allow buildGeometry to record multi-part features if they are present? (similar to fill: if you change it while recording we can include it; if it was set before recording we maybe don't need to?)