-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support calculating normals for indexed meshes #3987
Conversation
112299f
to
092eb7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for this PR 💯 -- I had a homebrew branch that did something similar, I've now changed my hobby project to use this PR. Hoping this will be merged.
I had some nit feedback
092eb7e
to
02cefbc
Compare
thanks for the feedback @mcobzarenco - I've updated the patch with both suggestions. |
The Doc comments to compute_flat_normals say: |
Do you have a backtrace that shows where it panics for you? If you're running with this patch then 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. |
Ah, I see, this Issue is Open, thus not merged in bevy 0.7.0! 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? |
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:
I think the docs for 0.7 saying "Panics if [ 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 If you need to use 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 |
It looks like At first glance it doesn't look like there was any other functional change to Certainly a backtrace could help clarify what assertion failure / panic your hitting though, if you're able to get one |
@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. |
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. 🤔 |
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.
02cefbc
to
ef27fa8
Compare
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 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. |
I checked its behaviour in my project and it seems to be doing what it is supposed to do. :)
...is not that developer friendly. :D |
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 |
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. |
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 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. |
Thanks for testing it out
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? |
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 That's why I'm proposing to keep it and just call it from a new 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. |
# Objective - Finish #3987 ## Solution - Rebase and fix typo. Co-authored-by: Robert Bragg <robert@sixbynine.org>
merged #11654 adds the feature, so please close this PR |
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 tobe 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.