Page MenuHome

Sculpt Code Breaks With NaN Vertices
Needs RevisionPublic

Authored by Joseph Eagar (joeedh) on Oct 15 2020, 11:51 PM.

Details

Summary

The pbvh code breaks if it encounters vertices with NaN coordinates. Sculpt tools stop working and the mesh randomly disappears in weird, glitchy ways.

This patch just checks for NaNs in verts when building a mesh pbvh (it doesn't do anything for bmesh or grids).

Anyway, the nans came from the multires reprojection code. I've been putting off fixing reprojection for years, but I think maybe this has motivated me to do something about it.

Diff Detail

Repository
rB Blender

Event Timeline

Joseph Eagar (joeedh) requested review of this revision.Oct 15 2020, 11:51 PM
Joseph Eagar (joeedh) created this revision.

What do you exactly mean with multires reprojection?
I know that the sculpt code breaks if it has NaNs (not only the PBVH building, but rendering and all brushes as well), but I'm not sure about checking for NaNs here. When people report bugs caused by a brush creating NaNs, I usually add the check in the brush before writing an invalid coordinate to the vertex. Also, is this other modes that use meshes are also checking when starting?

I meant the reprojection that happens in edit mode within bmesh, not anything to do with sculpt. I've committed a new branch for this; I thought it would be small enough to just submit a patch, but it looks like I'll need to clean up the multires code a bit. It shouldn't be too big of a change, just need to prune some old direct usages of CCGSubsurf (which shouldn't be used for multires as it's not compatible with what OpenSubdiv gives us).

Here's some history of what happened. So, ten or eleven years ago I wrote code to reproject multires grids when topology changes in edit mode. However, the function I wrote to convert multires displacements didn't work if the multires level wasn't set at its maximum. Consequently, when Campbell merged in the bmesh branch he disabled it, and I guess at some point someone modified the reprojection code to try and work in tangent space. But that doesn't really work since "tangent space" for multires depends on the underlying subdivision surface.

Anyway, I've decided to fix this whole mess. I'm sick of multires being useless to me because of bugs in bmesh I should really have fixed myself an embarrassing number of years ago.

Sergey Sharybin (sergey) requested changes to this revision.Oct 19 2020, 10:14 AM

Please make sure your IDE is configured for the clang-format.

Ignoring non-finite vertices when building PBVH is probably fine, but PBVH builder must not modify the mesh.
There are many more tools, operators and such which can be unhappy with non-finite vertices. So the proper fix is to avoid those vertices from being created in the first place.

@Brecht Van Lommel (brecht), adding you as a second thought about whether we need finite check in the PBVH, similar to the old own implementation of BVH in Cycles. To me it's not that important than fixing the root cause of non-finite vertices.

This revision now requires changes to proceed.Oct 19 2020, 10:14 AM

I've configured my IDE for clang-format. Anyway, like I said the NaNs were produced by buggy multires code in bmesh. I'm just about ready to submit a patch to fix that. I still think we should handle NaNs gracefully; it's not the fault of artists when buggy code produces them.

@Brecht Van Lommel (brecht), adding you as a second thought about whether we need finite check in the PBVH, similar to the old own implementation of BVH in Cycles. To me it's not that important than fixing the root cause of non-finite vertices.

I'm not sure we should add it. We have so far not automatically fixed these kinds of things, also not on file load, when entering edit mode, etc.

I'd rather not arbitrarily do it when entering sculpt mode to fix a specific issue, there should be some principle behind it.