Provide ability to disable feature adaptive subdivision, this disables snapping to limit surface and brings back iterative results. Best description and reasoning can be found in this (not mine) post: https://devtalk.blender.org/t/subdivision-surface-in-blender-2-8-behave-different-compared-to-maya-and-zbrush/12248/10
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
Boundary interpolation is fine with me, but it needs to be added to both Subsurf and Multires.
Also follow the feedback and make subsurf-flag-to-subdiv-conversion local to the modifier.
The feature adaptive I do not agree should be disabled. I've already wrote why is it so in another discussion at the devtalk. To summarize:
- There is a bug in OpenSubdiv's face-varying evaluator. This makes it so UVs are all messed up once you go uniform subdivisions. Surely, this is to be reported and fixed in OpenSubdiv, but because of other points it was not a priority so far.
- Multires must be feature-adaptive. Without this the artifacts which happens when changing subdivision level will be re-introduced. Long story short: feature-adaptive is a must-have for reliable multires. Having multires and subsurf behaving different seems odd to me.
- Other features do use feature-adaptive subdivisions. Cycles's adaptive subdivisions, USD/Alembic (to my knowledge), LANPR.
- Uniform subdivisions complicate GPU integration a lot, especially with smooth normals.
| source/blender/blenkernel/intern/subdiv.c | ||
|---|---|---|
| 75–88 | This needs to be in the MOD_subsurf.c. This is not a re-usable function, which is worsen with use of type-less input value. | |
Comment Actions
- There is a bug in OpenSubdiv's face-varying evaluator. This makes it so UVs are all messed up once you go uniform subdivisions. Surely, this is to be reported and fixed in OpenSubdiv, but because of other points it was not a priority so far.
That's indeed a bummer.
- Multires must be feature-adaptive. Without this the artifacts which happens when changing subdivision level will be re-introduced. Long story short: feature-adaptive is a must-have for reliable multires. Having multires and subsurf behaving different seems odd to me.
They cover different enough use cases, that it may not be best idea to force feature parity.
- Other features do use feature-adaptive subdivisions. Cycles's adaptive subdivisions, USD/Alembic (to my knowledge), LANPR.
They also require subdiv to be last in the stack, don't they? So there are already checks needed to see if mod stack is in supported state.
- Uniform subdivisions complicate GPU integration a lot, especially with smooth normals.
I'm sure you're right, but no other "modelling" modifier runs on GPU also. And I think this is core of the issue.
Currently Blender does not have capability to apply finite amount of Catmull-Clark iterations to a model, in modifier or otherwise.
Latching this on to existing modifier seemed to be most straightforward.
Maybe there should be separate modifier, lets call it "Subdivide" to cater to those needs, without ambition or promises of gpu support? (Still using OSD for creasing, and still bitten by UV bug for now)
Comment Actions
Sure, just asking about possible course of action after UV issue gets fixed eventually.
| intern/opensubdiv/internal/evaluator/evaluator_impl.cc | ||
|---|---|---|
| 252 ↗ | (On Diff #27334) | This is interesting, and raises many questions :) Why there are other levels? The generateIntermediateLevels is initialized to is_adaptive, so the stencil does not have intermediate levels. Where do they appear in the table? Is it really an intended behavior, or is it a bug when table is created for stencil? Or there is something missing in the table configuration? |
| intern/opensubdiv/internal/evaluator/evaluator_impl.cc | ||
|---|---|---|
| 252 ↗ | (On Diff #27334) | From what I gathered, It has to do with how Refine method, and stencil generation works. In adaptive mode src_face_varying_data_ contains values for coarse vertices followed by number of blocks for intermediate subdivision levels, and this works fine, apparently it's what OSD expects. Is this intended behavior it's hard to tell for me right now. I would imagine that if control points are expected in refined buffer for adaptive mode, then there will be "identity" stencils in the table to just copy them so the user is not forced to use "concatenated input output" technique we see in Refine. But maybe there is a good reason to do this this way, I find OSD documentation somewhat lacking in such details. |
Comment Actions
Think the proper plan of attack would be:
Split this patch into two:
- Expose boundary interpolation option.
- Expose limit surface setting for subsurf modifier.
The boundary interpolation needs to be added to both subsurf and multires. The limit surface is only for the usbsurf.
The explanation you did for the UV evaluation needs to be added to the comment in the code, so it's always easily accessible.
While splitting the patch might seem like unnecessary, it does help revisiting and revising changes later on.
| source/blender/makesrna/intern/rna_modifier.c | ||
|---|---|---|
| 1841–1845 | Call the "Use limit surface". Also don't think "snap" is the best verb here, and there is no explanation of what limit surface is for users. Would suggest something like "Put vertices at the limit surface (smoothest possible object)". Something like this. | |
| source/blender/modifiers/intern/MOD_subsurf.c | ||
| 159–161 | static. Also not sure about _from_bi. The bi part sounds a bit weird, and redundant. Maybe subdiv_vtx_boundary_interpolation_get is explanatory enough? | |
Comment Actions
- Remove boundary interpolation option
- Change user facing labels.
- Add code comments about UV fix.
- Change title and description to reflect more limited scope of patch.
.
Comment Actions
I think the changes in makesrna got lost? :(
I also think that RNA should be called use_limit_surface, as it is what scripters are facing.
For the flag, I suggest have "inverted" meaning in the enum, so that we don't need to bother with versioning code. It is still possible to expose as "use_limit_surface" by using RNA_def_property_boolean_negative_sdna.
| intern/opensubdiv/internal/evaluator/evaluator_impl.cc | ||
|---|---|---|
| 237 ↗ | (On Diff #27410) | I'm not sure why there is a comment about output is on the same line as src_. I would also suggest putting it as a non-inline argument, and let clang-format to take care of wrapping (otherwise it will be less readable comment after wrapping). |
| source/blender/makesdna/DNA_modifier_types.h | ||
| 159 | eSubsurfModifierFlag_UseLimitSurface Or, maybe invert the meaning of the flag, so it enables the "recursive" subdivision. Benefit of such "inverted" flag meaning would be that we don't need any versioning code. | |
Comment Actions
From quick look seems fine.
One thing I'm on a split here is this separation between "regular" and "advanced" subpanels. To me the subpanels should not exist, but UI team have different opinion. So not sure if this new option is something to be promoted as "regular" or "advance". Anyone have strong opinion?
@Brecht Van Lommel (brecht), do you want to have a glance here as well?
Comment Actions
The suggestion was to name the property use_limit_surface and RNA_def_property_boolean_negative_sdna to flip it internally. Any reason use_recursive_subdivision was used instead? I think that's a bit less clear, it's generally more user friendly to name options by the result they produce rather than the algorithm that is used.
I think this should be in the advanced subpanel. I don't expect this option to be used more than e.g. the levels settings, so I think it should be lower in the user interface, and I think it makes sense right above the quality setting.
Comment Actions
Reasoning behind putting this option near the top was that it has significant influence on modifier output similar to "Simple" and "Catmul-Clark"
Comment Actions
To me this seems fine (with the inlined comments).
@Brecht Van Lommel (brecht), such option is something what existed in the early subdivision surface code in 2.80. Do you still think it's fine/useful to have it for interoperability purposes?
| source/blender/modifiers/intern/MOD_subsurf.c | ||
|---|---|---|
| 466–468 | I think general rule was to gray-out sliders, not to hide them. @Julian Eisel (Severin), summoning UI mafia. | |
Comment Actions
Ugh, didn't see there are changes request from my side.
@Julian Eisel (Severin), do you mind having a pass of UI here?
| source/blender/makesrna/intern/rna_modifier.c | ||
|---|---|---|
| 1880 | Avoid using the term "limit surface" to describe itself. I think it can be explained fairly well with relatively few words. | |
| source/blender/modifiers/intern/MOD_subsurf.c | ||
| 466–468 | Yeah, please gray this out with uiLayoutSetActive instead (or uiLayoutSetEnabled() if there's a reason to disallow edits in the grayed-out state). | |
Comment Actions
- merge master
- disable, instead of hide, quality settings in non-adaptive mode
- change description of the toggle as per request
@Sergey Sharybin (sergey) @Julian Eisel (Severin) Description that I came up with is still kind of mathematical, it's difficult for me to think about this in non math way. Please advise.
| source/blender/makesrna/intern/rna_modifier.c | ||
|---|---|---|
| 1880 | How about "Place vertices at the surface that would be produced with infinite levels of subdivision (smoothest possible object)"? | |
| source/blender/modifiers/intern/MOD_subsurf.c | ||
| 467 | We typically use terms like col, row or sub for sub-layouts. Here I'd just use col, for the sake of consistency. | |