add InfallibleMesh for mesh construction without unwraps

[robtfm]

Objective

• implement the InfallibleMesh builder type suggested by @greeble-dev in Retain asset without data for RENDER_WORLD-only assets #21732 (review)
• remove all the try_xxx methods from Mesh, make the base methods return Results

Solution

builds on #21732 - could replace that, but i thought better to raise a different PR to keep the changes separated as this touches quite a lot more of the codebase. to see the diff for just this pr i created https://github.com/robtfm/bevy/pull/5/files which applies this onto the original pr.

• add InfallibleMesh type for use during cpu-side mesh construction, which wraps Mesh and ensures that the internal mesh data can't be extracted.
• modify primitives and examples to use the new type
• bonus: clean up the From<primitives> for Mesh code: remove ~30 individual From<T> for Mesh implementations and replace with a blanket impl, supported by modifying Meshable::mesh to take self instead of &self. basically all existing implementations were copying or cloning self anyway, so this doesn't seem like it should cause any issues.

Changelog

Existing accessor methods on Mesh have been changed to return a Result<T, MeshAccessError> which will return an error if the Mesh has been extracted to the RenderWorld.
When constructing meshes we now recommend using the InfallibleMesh type which guarantees the internal data cannot be extracted prior to being converted into a normal mesh.

[greeble-dev]

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.

[robtfm]

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.

[greeble-dev]

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.

[robtfm]

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?

[greeble-dev]

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.

[robtfm]

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.

[greeble-dev]

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.

[beicause]

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.

[greeble-dev]

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.

[beicause]

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.

[greeble-dev]

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.