Support calculating normals for indexed meshes

[rib]

Currently the PBR renderer requires every mesh to have a normals attribute, and in case normals are missing there was a compute_flat_normals() utility.

This utility was documented to panic in case a mesh was based on indices and so to avoid that there is also a utility to expand a mesh into one that doesn't depend on an index buffer.

To avoid needing to expand meshes to avoid index buffers, this updates the utility to be able to calculate normals even for indexed geometry.

Note: this still has the other notable limitations that it assumes a triangle list topology and will panic for triangle strips or meshes that don't have vec3 float32 position attributes.

The api was renamed to compute_normals() since they can't be assumed to be flat in the case that indexed vertices may be shared by multiple faces. The calculated normals are (naively) averaged without any weighting, such as by the angle or area of each adjacent face.

The gltf loader has been updated to no longer expand an indexed mesh via duplicate_vertices before generating missing normals.

Ideally in the future there will be some kind of separation of how assets like meshes are imported into bevy where there will be a natural place to configure exactly how missing normals should be calculated, including choosing what weighting to use for averaging.

[rib]

thanks for the feedback @mcobzarenco - I've updated the patch with both suggestions.

[DerKarlos]

The Doc comments to compute_flat_normals say: Panics if [Indices] are set Did this change? In my code, it panics if Indices are NOT set.

[rib]

Do you have a backtrace that shows where it panics for you?

If you're running with this patch then compute_flat_normals is now named compute_normals and it should work with or without indices (in the case without indices it should run the same code as before)

Other (pre-existing) limitations of this code to be aware of are that the position attribute must be float3 format and must have a triangle list topology.

[DerKarlos]

Ah, I see, this Issue is Open, thus not merged in bevy 0.7.0! I use compute_flat_normals. If I use it before set_indices, it crashes. So the doc of 0.7.0 is wrong, right? I could make a backtrace. Shall I?

I don't know GitHub that well: If you like, I may test your "force-pushed"? How could I get/build your/that bevy version?

[rib]

Right, it looks like my patch didn't make it into 0.7 unfortunately.

To test my branch you could potentially add something like this to your Cargo.toml to get bevy from my git branch instead of crates.io:

[patch.crates-io]
bevy = { git = 'https://github.com/rib/bevy.git', branch = 'wip/rib/calc-indexed-normals' }

I think the docs for 0.7 saying "Panics if [Indices] are set" are correct for compute_flat_normals. The first line of the function is this:

assert!(self.indices().is_none(), "`compute_flat_normals` can't work on indexed geometry. Consider calling `Mesh::duplicate_vertices`.");

To avoid that assertion code that needed to call compute_flat_normals would first call mesh.duplicate_vertices() to force any indices to be unpacked and give a larger mesh that doesn't depend on indices.

If you need to use compute_flat_normals directly yourself on 0.7 then yeah, unfortunately, you'll also need to call something like mesh.duplicate_vertices() to ensure your mesh doesn't use indices.

It sounds odd though that you're seeing some kind of panic in a case where you don't have indices... I haven't checked if the original code has changed since I wrote the patch and maybe it was re-written since I last looked at it (I see github is highlighting that the patch has conflicts now) - maybe another change on master (for 0.7) has introduced an issue you're hitting.

[rib]

It looks like set_attribute was renamed to insert_attribute which is the source of the conflict here.

At first glance it doesn't look like there was any other functional change to calculate_flat_normals since I made this patch but it's worth noting that @cart did land a significant change (e369a8ad5138af28a7e760fac3f07b278c27ebb4) related to supporting various vertex buffer layouts which might be relevant here (though I haven't had a chance to investigate that change yet so can't really suggest anything specific to try)

Certainly a backtrace could help clarify what assertion failure / panic your hitting though, if you're able to get one

[IceSentry]

@rib Can you rebase and fix the conflicts? The math looks right to me, I wanted to make a PR with something very similar and I saw yours.

[tkgalk]

Any updates on this one? Has this lost a maintainer? I'd love to see this in the 0.10.0/0.11.0. Would be pretty useful to me and I'm sure to others as well. 🤔

[rib]

heya, sorry that looking at this hasn't been a priority for me recently. I've just done a rebase which I smoke tested with cargo run --examples load_gltf but I haven't actually re-tested the compute_normals code.

It would be good if someone (maybe @galkowskit and/or @IceSentry ?) could give the updated branch a try and check that it's working as intended.

[tkgalk]

I checked its behaviour in my project and it seems to be doing what it is supposed to do. :) I think the only nitpick I might have is that it will panic and crash if it is called on a mesh without any indices or vertices added. It would be great if it crashed, but with some error message stating why.

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value'

...is not that developer friendly. :D

[IceSentry]

So, one thing I don't like about this PR is that it gives flat normals on non-indexed meshes and smooth on indexed meshes.

I think we should keep the compute_flat_normals() as is, but also have a separate compute_smooth_normals(). Then also have a compute_normals() that just calls the appropriate one. This should all be documented appropriately too.

[IceSentry]

Also, can you update the PR description by using the default PR template? It makes it much easier to generate the release notes and migration guide when the template is used.

[rib]

Just to let you know - I updated the PR since it was requested a few times but I have to admit this isn't a priority for me currently so I don't imagine I'm going to have time to make changes (since I'm also not in a good position to test changes currently), sorry.

There are certainly lots of things that could be improved about how Bevy handles various meshes missing data, but I'd also note that this PR was only really intended to make a single specific improvement to support (basic) generation of normals for indexed meshes (that are triangle lists). (I.e. don't panic and generate some reasonable normals)

I'm not so sure it's really necessary here to keep a compute_flat_normals and add a compute_smooth_normals - the renamed function specifically doesn't specify "flat" any more because it's now context dependent. The new function is compatible with the old compute_flat_normals in that it generates "flat" normals for non-indexed geometry and it generates basic (averaged) "smooth" normals for indexed geometry.

A more sophisticated solution would do a lot more - including allowing overwriting pre-existing normals, generating smooth normals for non-indexed meshes and supporting additional smoothing algorithms. This would presumably also be architected very differently; part of a pre-processing pipeline for assets, instead of something that's done on-the-fly at runtime like this since some of the algorithms for calculating normals may be relatively costly.

Please feel free to take this patch as a starting point and re-spin it as you like.

[rib]

I checked its behaviour in my project and it seems to be doing what it is supposed to do. :) I think the only nitpick I might have is that it will panic and crash if it is called on a mesh without any indices or vertices added. It would be great if it crashed, but with some error message stating why.

Thanks for testing it out

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value'

...is not that developer friendly. :D

As far as I recall the patch shouldn't introduce a panic for anything that didn't already panic - or are you maybe seeing that it introduces a new panic that didn't already exist?

[IceSentry]

To clarify a little what I meant. I understand that this PR is to make the current state a bit easier and not the final solution for all normal generation. I just think that there's no point in removing the current compute_flat_normals() because sometimes you just want to compute flat normals and hiding it behind a function that may or may not compute flat normals depending on the given mesh is a bit of a convoluted way to achieve the intended results. Especially given that the api already existed.

That's why I'm proposing to keep it and just call it from a new compute_normals() and at that point might as well have a compute_smooth_normals().

Also, just to add, asset preprocessing is in the works, but runtime normal generation is still really useful, for example for procedural generation, and should stay there.

Anyway, since you can't easily update the PR, I'll tag it as Adopt-Me.