Page MenuHome

Cycles: Stitching of subdivided and displaced meshes
AcceptedPublic

Authored by Mai Lavelle (maiself) on Sep 14 2018, 4:46 AM.

Details

Summary

This patch stitches the vertices along patch edges so that cracks can no longer form when applying subdivision or displacement a mesh.

Subpatches are now formed in a way that ensures vertex indices along subpatch edges are equal for adjacent subpatches. A mapping of vertices along patch edges is built to preform stitching. Overall performance is roughly the same, some gains were made in splitting, but some was lost in stitching.

This fixes:

  • T49049 (cracks between patches from material and uv seams)
  • T49048 (discontinuous normals with true displacement)

Diff Detail

Event Timeline

Ronny G (nutel) added a comment.EditedSep 14 2018, 9:13 AM

2 examples

baked displacement map

2 materials with different displacement settings

Sergey Sharybin (sergey) requested changes to this revision.Sep 14 2018, 10:23 AM

While the functionality sounds good and useful, i am not happy with the technical implementation:

  • Don't use auto. It is only for lazy developers, it does not help readability at all.
  • Use primitives from ccl, if they do not exist there -- consider adding them. While this might sound like extra work at this exact moment, this helps a lot in the future when we decide to tweak performance/add extra guards.
  • Stick to Cycles comment style, which uses C comments. Also, sentences starts from capital and ends with a full stop. Sentences in comments are not special in this regard at all.
  • Don't use #pragma once, we use header guards.

And in general: stick to existing code style, code templates and such. That is way more important for keeping readability/maintainability of Cycles in a long term than saving few key presses at a moment you've typed the code once.

intern/cycles/render/mesh_displace.cpp
29

Use util_set.h, we do have unordered_set in ccl namespace already.

51
  1. I am absolutely not a fan of auto. That might allow you to type 5 character less, but readability goes down by a factor of a lot. Really, how would one guess what i.second is in this case?
  2. Stick to foreach for until there is a decision to remove that.
55–56

Same as above.

And all auto cases below.

intern/cycles/render/mesh_subdivision.cpp
482

Move all this code to a helper function.

intern/cycles/subd/subd_dice.cpp
65–73

Remove dead code.

76–84

Same as above.

intern/cycles/subd/subd_split.cpp
162–163

Add an assert if we really shouldn't get here.

177–211

Use anonymous namespace rather than struct declaration nested to a function.

246

Same as above.

448

What is auto&. Also, seems you don't intend to modify edge => use const qualifier.

intern/cycles/subd/subd_subpatch.h
18

We are using explicit header guards.

118

What is T?

129

We do have pair in ccl. Also, in such cases comment what are the values. key is not giving any inform,information here.

This revision now requires changes to proceed.Sep 14 2018, 10:23 AM

Great to see this tackled!

Let's follow the code style as @Sergey Sharybin (sergey) says, but otherwise I see no major issues with this patch.

Mai Lavelle (maiself) marked 12 inline comments as done.Sep 14 2018, 11:16 PM
Mai Lavelle (maiself) updated this revision to Diff 11863.

Attempted to address all style issues.

intern/cycles/subd/subd_subpatch.h
18

I'm fine with this, but looks like there's several headers with this already.

From code side seems fine now. Didn't have time to run tests or anything, so relying on you here ;)

Maybe add simple scene to regression set.

This revision is now accepted and ready to land.Sep 24 2018, 5:59 PM

This was never committed to master due to a crash discovered at the last minute. The method used here assumes that the T value calculated on each edge is the same for both sides of the edge. While this seems reasonable, there are apparently some uncommon cases where this assumption is wrong. I'm investigating weather the current Diagsplit implementation can be made to ensure that both sides produce the same value, otherwise an alternate approach will be needed.

  • Rebased on master
  • Added an assert to line 277 of subd_split.cpp to mark where things can start to breakdown.

This was never committed to master due to a crash discovered at the last minute. The method used here assumes that the T value calculated on each edge is the same for both sides of the edge. While this seems reasonable, there are apparently some uncommon cases where this assumption is wrong. I'm investigating weather the current Diagsplit implementation can be made to ensure that both sides produce the same value, otherwise an alternate approach will be needed.

That sounds like a bug in the diagsplit implementation. Just a guess, maybe some computation happens with the two vertices of the edge in different order, and ordering them based on vertex index helps.

I think this should fix it. I was able to render a significantly complex scene after making these changes, but more testing would be good.

The main problem was the implementation was splitting too aggressively when one edge was marked for nonuniform tessellation and the opposite edge was too short. Now instead of continuing to split when splitting should stop, the T value will be calculated as if continuing to split, without actually doing so. This should keep T equal on both sides of an edge regardless of how much splitting is actually done on either side.

  • Fixed all known crashes.
  • Added some extra asserts.
  • Added ordering to points to avoid possible numerical precision issues, tho I'm not sure how necessary this is.

Updating with diff against master instead of intermediate commit as done by accident in last update...

  • Removed vertex ordering from partition_edge, this caused incorrect stitching.
  • Silenced warning about unused variable.
Mai Lavelle (maiself) updated this revision to Diff 17324.EditedWed, Aug 21, 2:17 AM

Fixed some more cases where T could vary. In particular, edge factor limits weren't applied during recursive resolve of T.

  • Moved application of edge factor limits into T calculation function.
  • Fixed limits on ngon edge factors that where wrong (by factor of 2) due to initial split.
  • Ensure first two splits for quad patches always happen, regardless of any limits.

Also discovered a bug where Blenders viewport subdivision is unnecessarily applied in addition to Cycles subdivision. Haven't looked into cause or solution yet, but unrelated to this patch.

(Cause related to this todo? https://developer.blender.org/diffusion/B/browse/master/intern/cycles/blender/blender_util.h$52 )

Yes, the double subdivision is due to depsgraph changes in 2.80. We don't yet have a solution for this.

Probably it will be best to have the option to use adaptive subdivision as a Blender property recognised by the dependency graph. Then for any renderer supporting its own subdivision, the dependency graph would skip the subdivision surface modifier for rendering if it's last in the stack. Also related to T68891: Subdivision surface settings part of the Mesh.