Page MenuHome

Expose OpenSubdiv boundary interpolation options.
Needs ReviewPublic

Authored by Piotr Ostrowski (postrowski) on Aug 6 2020, 4:25 PM.
Tokens
"Like" token, awarded by Thane5."Like" token, awarded by MetinSeven."Love" token, awarded by lcs_cavalheiro."Like" token, awarded by Fracture128."Like" token, awarded by EAW."Like" token, awarded by APEC.

Details

Summary

Expose VTX_BOUNDARY_EDGE_AND_CORNER interpolation option, this makes easier to block out curved surfaces without needing to keep boundary edge crease at 1.0

Diff Detail

Repository
rB Blender

Event Timeline

Piotr Ostrowski (postrowski) requested review of this revision.Aug 6 2020, 4:25 PM
Piotr Ostrowski (postrowski) created this revision.

@Sergey Sharybin (sergey) function subdiv_vtx_boundary_interpolation_get is duplicated, please advise, where to put it so both modifiers can use it.

Yeah, you're right, and this is a bit annoying. Some of the flags are duplicated between Multires and Subsurf, which makes it easy to localize in the modifier implementation file, but doesn't allow to so easily share code for common bits.

Your original take on it seemed better, but I would do some naming adjustments. So how about add this next to BKE_subdiv_fvar_interpolation_from_uv_smooth ?

/* NOTE: boundary_interpolation is  eSubsurfBoundaryInterpolation. */
eSubdivVtxBoundaryInterpolation BKE_subdiv_vtx_boundary_interpolation_from_subsurf(int boundary_interpolation);

It states what enum is expected to be. Still not sure about suffix: subsurf is too similar to subdiv, but i think is a bit more clear from the original patch which used bi (which doesn't imply the nature of the source of the value).

But there is one more serious issue with the current implementation: it will not preserve compatibility.
Not sure if dna_defaults.c can be used to easily initialize the flag for existing .blend files. Alternative would be to either:

  • Add code to do-versions to initialize boundary interpolation to SUBDIV_VTX_BOUNDARY_EDGE_ONLY. This is annoying because you need to more code.
  • Use value of 0 for SUBDIV_VTX_BOUNDARY_EDGE_ONLY. This is annoying because order of values is not "canonical".

If you have strong about any of possible ways, go for it. Otherwise we flip a coin ;)

source/blender/makesrna/intern/rna_modifier.c
1778–1779

We use /**/ style of comments. Also consider adding a fullstop at the end of the sentence ;)

  • Add versioning
  • Move converter to common place
  • Comment formatting.

To me this seems fine.

@Brecht Van Lommel (brecht), do you want to have a look here?

This revision is now accepted and ready to land.Aug 11 2020, 1:57 PM
Brecht Van Lommel (brecht) requested changes to this revision.Aug 11 2020, 4:56 PM

What this does is effectively give you "smooth corners" or "sharp corners", which are easier to understand names. If this option exists for specifically for compatibility then it would make sense to follow standard naming, but I haven't check how it's named in other apps.

source/blender/makesrna/intern/rna_modifier.c
1752

only -> Only

1752

Add description

1756

vertices -> Corners

This is specifically about corners, not all vertices.

1757

Add description

1773

interpolation -> Interpolation

This revision now requires changes to proceed.Aug 11 2020, 4:56 PM

Thanks for the rebase.
@Piotr Ostrowski (postrowski), not sure from reading, is all the feedback from Brecht addressed now?

Yup. Brecht's comments were only about naming and missing description.
Those are fixed now.