I've skimmed the changes and couldn't spot any obvious issues.

I'm not sold on the way InfallibleMesh contains a Mesh and can deref to &Mesh. I think that constrains the implementation as it can only use data that's in Mesh, and every mutation has to make sure Mesh is in a valid state. Maybe that becomes a limitation if we want to support more expensive transforms that only happen when everything has finished - stuff like compression. So I'd rather all the mutating methods moved out of Mesh, and InfallibleMesh was opaque and the only way to get at the finished Mesh is via From<InfallibleMesh>.

But that's debatable, and the current version does solve an immediate problem. So maybe it's fine to get the current version in first, and make the case for follow on changes separately.

That would mean having InfallibleMesh as a separate asset type? I can see lots of issues arising from that, on first glance I’m definitely on the opposite team for the debate…

In case it helps with framing, I would have called it MeshBuilder if that wasn’t already taken.

No, I didn't intend for InfallibleMesh to be a separate asset type. It's just a temporary helper. So the only change to the current PR would be to remove the implicit deref - then InfallibleMesh can only be turned into a Mesh through Into<Mesh>. Maybe in the long-term the InfallibleMesh internals move away from Mesh. Or maybe they stay as they are.

Actually... just checking the PR - InfallibleMesh currently derives Asset? Is that intended? Feels wrong regardless of which approach is taken.

the reason for the deref was primitive builders that use mesh_a.merge(&mesh_b), where merge takes a mesh. there are alternative ways it could be managed (InfallibleMesh::merge could require another InfallibleMesh to merge with, though that seems unnecessarily restrictive).

we only allow read-only access to the mesh content though, so that we can still ensure it hasn't been extracted.

i don't think i've quite understood the point about compression though - if it modifies the mesh and we don't want it to, we just avoid providing an InfallibleMesh compression method ... but maybe that's not what you meant?

The scenario I'm imagining is that Mesh becomes more optimised for loading and GPU upload at some point in the future - maybe that's through compression, or interleaving attributes, or packing everything into a single allocation. Or maybe there's extra validation added to make sure attributes match up. These kinds of things are difficult to do incrementally as attributes are added - they want the attributes finalised first so they can validate or optimize everything in a single step.

If InfallibleMesh can be deref'd to Mesh at any time then that could be difficult. It would be easier if the only way to get a Mesh value is through calling Into<Mesh> (or some other function), so it's clear that the user is done adding attributes and any validation/optimization can now be done.

But I'm just speculating. Maybe the API ends up looking completely different.

you're right about the Asset derive - removed, thanks.

regarding invalid mesh states, i see what you're saying now, but agree we can cross that bridge when we come to it.

Sounds good to me. Personally I'd be minded to approve this PR even though I'm not keen on the deref - it's still a step in the right direction.

The scenario I'm imagining is that Mesh becomes more optimised for loading and GPU upload at some point in the future - maybe that's through compression, or interleaving attributes, or packing everything into a single allocation. Or maybe there's extra validation added to make sure attributes match up.

@greeble-dev 😅I realize that is a bit difficult and greatly breaks compatibility. Because bevy_picking or the physics engine may read the vertex data of the Mesh component so we cannot store only the GPU optimized interleaved vertex attributes on the Mesh. I would rather treat the Mesh as a component that is generic for both CPU and GPU, and add a separate GPU-friendly “raw” mesh component in some way (not sure if it’s feasible). In that case, I’m not sure if InfallibleMesh still makes sense.

My guess is that there'll be pressure to keep Mesh3d as struct Mesh3d(Handle<Mesh>), so users who just want to put scenes together are insulated from the complexity - whether it's optimized or not it's always a Handle<Mesh>. And bevy_picking and physics engines would be expected to deal with that complexity. Only speculating though.

Iterating over compressed or interleaved vertex attributes might degrade mesh performance for CPU reads, not the complexity of handling it. If the goal is just to have a mesh that’s efficient for GPU uploads, a mechanism like Mesh3d<Handle<Mesh>> -> RawMesh3d<Handle<RawMesh>> -> Rendering makes more sense to me.

I think the current Mesh interface is fine. Even modifying it can panic if it's extracted, I guess most users would still use Mesh over InfallibleMesh.

In my view, the key issue is separating the mesh types optimized for writing from the mesh types optimized for reading.

I'm also assuming there would be multiple types optimised for different kinds of reads - so having physics be able to read GPU mesh data is a useful convenience in some situations, but for situations where performance is more important there would be a separate type optimized for physics.

So I see this PR as a step forward to separating read and write types. I'm not so worried about which of those types ends up getting called Mesh.

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

if we changed InfallibleMesh to MeshBuilder, we could logically change MeshBuilder to MeshBuilderBuilder ... or maybe IntoMeshBuilder

Personally, I think this is controversial and am not convinced about this PR:

1. It introduces a large amount of duplicate APIs for InfallibleMesh and Mesh. Subsequent adding of more Mesh APIs requires duplication every time. This is a maintenance burden.

2. Building a Mesh is already impossible to panic. And I feel removing the panic versions from Mesh is not good enough. For example, user code still need multiple checks or unwraps for Mesh. IMO code quality and Mesh access ergonomics are the priority.

I'm adding the X-Contentious label. Partly because of disagreements. Partly because the longer term goals for meshes - interleaving, loading efficiency, assets-as-entities, etc - seem to be open questions and might be affected by the direction this PR is taking.

Similarly not super keen on the prospect of additional code duplication here, but I agree that the mesh ergonomics situation with unwraps is not great. I am content to largely defer to our assorted SMEs here though.

For what it's worth, my guess is that the duplication is mostly temporary. So in the future Mesh will become more of a read-only interface, and so a lot of with_xxx-like functions fall away. But that's just a guess as we don't have a roadmap.