Page MenuHome

Freestyle: new stroke modifiers
ClosedPublic

Authored by Folkert de Vries (flokkievids) on Dec 31 2014, 5:25 PM.

Details

Summary

This patch introduces a couple new stroke modifiers. The ones currently implemented are based on prototypes by @Tamito Kajiyama (kjym3) and myself.

The new modifiers:

  • Tangent
  • Thickness noise
  • Crease Angle
  • Simplification
  • Curvature 3D

The documentation for these new modifier types can be found in the manual:


(left: AnisotropicThicknessShader, right: NoiseThicknessShader)


(left: Curvature 3D, right: Simplification)

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
release/scripts/freestyle/modules/parameter_editor.py
1332

This line of dead code should be removed.

release/scripts/startup/bl_ui/properties_freestyle.py
419

Is this comment still valid?

422

The "pass" statement can be removed.

426

Put the two properties in one row to save space.

442

Swap the two rows of parameters (thickness range and Kr range) to make the UI layout consistent with other modifiers you've added.

source/blender/blenkernel/intern/linestyle.c
659

Angle initial values must be in radians. Use DEG2RADF (see its occurrences in the same file).

source/blender/makesrna/intern/rna_linestyle.c
957

The symbol Kr should be renamed to something more meaningful in the parameter names and their descriptions. I would suggest "curvature". (That is still quite technical, but we don't have much freedom here...)

As to the hack in the ThicknessNoiseShader, you can rewrite the shader from scratch without relying on the C++ implementation, if that approach turns out a better solution. If I remember correctly, I was asking you not to remove the C++ thickness noise shader for possible inclusion into the Parameter Editor. But implementing the modifier based on the C++ shader was just an idea and is not a requirement. Please, feel free to propose what you think is the best.

Thanks for the review,

I've fixed most issues already, and started work on implementing the thickness shaders as color and alpha ones. I'm really struggling, from a UI and artist perspective, with the input. For instance the Tangent shader maps a range (really range(0, 2pi) but in practice this is already range(0, 1); this doesn't help for clarity) to a color distribution as a color ramp. This is quite complex and, i fear, hard to communicate.

for the tangent alpha shader, following the current implementation, would require two curve mappings. one could suffice but this would again be unnatural, since this happens nowhere else in blender, as far as I know. (seeing real potential for the BEER 2D color ramps here) As far as code goes, this is all fine, but I'm not sure how I can make these tools the most intuitive for a user.

As to the 'hack': I tried to replicate the Noise shader in Python, but didn't really get a good result. It may be possible when the used noise functions are exposed to Python though.

furthermore I also fixed an inconsistency in the argument order for the new modifiers, they now all follow the original modifiers.

I believe the basic idea of mapping a scalar value to a color based on a color ramp is intuitive and straightforward to understand. A difficulty comes from the fact that the scalar value in the case of a tangent color and alpha modifiers is an angle between 0 and 360 degrees. A color ramp in the form of a circle could address that difficulty. Here is a mock-up (made using GIMP from a screen capture of Blender).

An associated issue is that the range of a color ramp and of a curve is hard-coded as distance (see rna_def_modifier_color_ramp_common() and rna_def_modifier_curve_common() in rna_linestyle.c). For tangent-dependent modifiers the unit of the range should be PROP_ANGLE. Some code refactoring may be needed to fix this issue (give a try if you like).

As to a noise generator accessible from within Python, freestyle.types.Noise could be an option. Since the noise modifiers will be something completely new in the Parameter Editor, we don't really have to care about backward compatibility.

Folkert de Vries (flokkievids) updated this object.
Folkert de Vries (flokkievids) edited edge metadata.

More shaders!

I've implemented color and alpha variants for the earlier presented thickness modifiers. Additionally, two more noise functions are exposed to freestyle.types.Noise. These new functions are used to implement the Noise* modifiers in Python (hacks begone! (actually turned out that the hack didn't work as I expected))

notes:

  • CurveMappingModifier: why use a special function (evaluateCurveMappingF) instead of self.curve.curves[0].evaluate(t)? seems unneeded.
  • what exactly is a curvature/Kr value? does it have bounds? (seems to go from 0 to .01 on a normal object, but I'm really not sure what's going on there. The curvature modifiers also still need better default values.
  • I've renamed the anisotropic shaders to Tangent*Shader, but maybe curvature 2D would suit better (symmetry)
  • ThicknessNoiseShader: the result looks much better when it is not symetrical. we could maybe create an option so that some shaders can assign to a specific side (r or l), still taking the blend_type into account.
  • ThicknessColorShader: I've also quickly implemented an alternate shade() method, but it doesn't seem very useful.
  • an irritating problem that I sometimes run into: the function applyShading in source/blender/freestyle/intern/stroke/Operators.cpp crashes when a custom strokeshader doesn't init the StrokeShader class. code in BPy_Operators seems to suggest that this shouldn't/doesn't happen, but it does.
  • the naming of functions that I've put in utils.py could maybe be better.

Still trying to tame my new linux setup...

I missed several whitespace issues and fixed some sorting and spelling here and there.

Hi flokkievids,

Please, find below my comments on the latest version of Patch D963 (new stroke modifiers).

  • CurveMappingModifier: why use a special function (evaluateCurveMappingF) instead of self.curve.curves[0].evaluate(t)? seems unneeded.

It's preferable to use self.curve.curves[0].evaluate(t). I guess when the original code was written the .evaluate() method was unavailable or I was unaware of it.

  • what exactly is a curvature/Kr value? does it have bounds? (seems to go from 0 to .01 on a normal object, but I'm really not sure what's going on there. The curvature modifiers also still need better default values.

A curvature of a curve at a point on it is a reciprocal of the radius of a circle that locally fits the curve at that point. Let r be the radius, then the curvature is 1/r. Intuitively, the curvature is large if the curve turns quickly. The flatter the surface is, the larger the curvature is.

For a 3D surface, curvatures are evaluated for a cross section of the surface and a plane. The variable Kr is used in Freestyle to refer to a radial curvature which is evaluated with a plane defined by the surface normal at the point and a camera position.

So, the range of Kr values is between 0 and infinite. A radial curvature on a unit sphere mesh is 1, so reasonable default values would be from 0.1 to 10.

  • I've renamed the anisotropic shaders to Tangent*Shader, but maybe curvature 2D would suit better (symmetry)

I would prefer the word tangent, since the Tangent shaders work on a different quantity than the curvature as mentioned above.

  • ThicknessNoiseShader: the result looks much better when it is not symetrical. we could maybe create an option so that some shaders can assign to a specific side (r or l), still taking the blend_type into account.

I agree and we can add an option to modify thickness in an asymmetric way.

  • an irritating problem that I sometimes run into: the function applyShading in source/blender/freestyle/intern/stroke/Operators.cpp crashes when a custom strokeshader doesn't init the StrokeShader class. code in BPy_Operators seems to suggest that this shouldn't/doesn't happen, but it does.

Thanks for the problem report. The missing call of StrokeShader.init(self) should be properly detected and a crash should not happen.

  • the naming of functions that I've put in utils.py could maybe be better.

To me the present function names are reasonably good and acceptable.

release/scripts/freestyle/modules/parameter_editor.py
510

Would be appreciated if there is a quick docstring for the new shaders, in particular to give users a clear idea of the technical term 'tangent'.

557

How about making the seed a user-tunable parameter?

576

You might want to consider some syntax sugar here to do what .blend_thickness() does by operator overloading or some other tricks. Proposals are welcome (:

597

This old implementation could be just removed.

731

t = self.Kr_min

733

t = self.Kr_max

release/scripts/startup/bl_ui/properties_freestyle.py
373

The Curvature 3D modifiers require the Face Smoothness option enabled (otherwise curvature values won't be computed and the modifiers do not work as expected). How about showing a warning message within the modifier windows in the UI if the Face Smoothness option is disabled? Another solution could be just to gray out the modifiers when that is the case and treat them as if they are muted.

source/blender/blenkernel/intern/linestyle.c
80

A better name for this one would be "3D Curvature" (i.e., three-dimensional curvature). This is a user-visible name. In the source code the swapped word order like CURVATURE_3D is just fine.

550

For curiosity: Is there a reason why 2pi was chosen for the default value of the amplitude?

source/blender/freestyle/intern/python/BPy_FrsNoise.cpp
106

The second and third arguments should be removed (because of the METH_NOARGS flag in PyMethodDef).

281

I would name this function simply as rand() and omit the suffix 48 because the use of the drand48 random number generator (RNG) is an implementation detail and it could be replaced with another RNG.

Also, how about adding an API function for RandGen::srand48() as well to give users full control on the RNG seed?

Corrections (inlined).

release/scripts/freestyle/modules/parameter_editor.py
731

This comment was wrong. I meant t = 1.

733

Ditto for this comment. I meant t = 0.

So 1 corresponds to Kr_min and 0 corresponds to Kr_max. I think the correspondences need to be swapped for consistency with other modifiers.

some questions (inlined). A first set of changes should be ready soon.

release/scripts/freestyle/modules/parameter_editor.py
733

I think I understand this.

the formula

t = (self.Kr_max - Kr) / (self.Kr_max - self.Kr_min)

gives that a higher Kr value will produce a smaller t, the statements above do the opposite (high Kr => t = 1)

Is this what you mean?

source/blender/blenkernel/intern/linestyle.c
550

No, not really. I just did some experimentation, and 2pi looked good. but any value between 5 and 10 would probably be a good default.
not quite sure how to choose a good number though.

some changes

  • added a base class for the Noise shaders. the amount of code duplication made this desirable
  • the drand48() function was renamed to rand() and takes an optional seed argument.
  • A (very ugly) implementation of asymmetric thickness assignment.
  • some stylistic changes (removing dead code, adjusting use of parentheses)

Thanks for the patch update. Here are a few comments (inlined) through code reading. I will find time to compile and test the code later.

release/scripts/freestyle/modules/freestyle/utils.py
130

We cannot assume any default Kr value when computed curvatures are not available. In that case, the function should tell the caller that Kr at the given stroke vertex is void and let the caller decide what to handle such situation.

136

Radial curvature Kr is supposed to be within the range from 0 to infinity, so abs() should not be necessary. Let me know if you have observed negative Kr values.

release/scripts/freestyle/modules/parameter_editor.py
733

Yes, that is exactly what I mean. Rethinking the mapping Kr and t, I would say the present formula is fine as well. When I made the previous comment (i.e., " I think the correspondences need to be swapped for consistency with other modifiers.") I though it would be easier-to-understand for users to have a proportional mapping (giving a smaller t for a smaller Kr) rather than an inversely proportional mapping as in the present formula. But Kr can be interpreted more intuitively by saying "a larger Kr means a flatter curve, whereas a smaller Kr implies a more quickly turning curve." In short Kr indicates local flatness at a point of a curve and hence of a surface. So a user friendly description of the mapping defined by the present formula would be: "A flatter surface gives a smaller t, while a more quickly turning surface gives a larger t." Does this make more sense than a conventional technical definition of radial curvature Kr?

745

The use of bound() looks good. The dead 'if' statement can be removed.

source/blender/blenkernel/intern/linestyle.c
550

Okay, then let us choose 10 as default, to make the effects of the modifier more visible.

A small question based on your comment

release/scripts/freestyle/modules/parameter_editor.py
733

yes, but don't miss the (1 - t) on line 738. the proportionality is inverted again there.

Intuitively, I would say that a flatter surface should be on the left side of the color ramp (so small t). I believe that the (1 - t) should be changed to just (t) to achieve this, right?

release/scripts/freestyle/modules/parameter_editor.py
733

Oh I was overlooking the inversion of the proportionality at line 738, thanks for the heads up.

I agree that intuitively the flatter surface (Kr_max) is on the left side of the color ramp (t = 0). That can be done by b = self.evaluate(t) as you said.

Using a very simple scene (just suzanne with a level 2 subsurf) already gives a negative Kr value:

AssertionError: Kr < 0, this is a bug. 
Kr value = -0.023580566258899045
c1 = (-0.17528972561248696, -0.006033688650196869, Vector((0.6879411339759827, 0.3717944622039795, 0.13567295670509338)), Vector((-0.41108763217926025, 0.4071488380432129, 0.8144540190696716)), -0.023580566258899045, Vector((-0.2623899579048157, 0.280418336391449, 0.8959897756576538)), 0.0)
c2 = None

So it seems that Kr can be below 0. Of course this value can just be abs'ed here, but based on the definition of curvature (1/r), this is really strange.

blend file to reproduce:

more changes after code review

  • added a more descriptive assertion error to curvature_from_stroke_vertex
  • expose the seed value for noise modifiers.
  • fix bugs in the noise modifers (not copying values and the alpha one didn't expose a curvemap)

As to curvatures, I reviewed the literature and learned both positive negative values are expected convex and concave curves, respectively. So my comment on applying abs() to Kr values was incorrect. My apologies.

We have two choices now. One is to apply abs() to Kr values. In this case we simply focus on the "magnitude" of surface flatness. Another choice is to let the 3D Curvature modifier negative Kr values (so the expected Kr range is from negative infinity to positive infinity). This might be useful since it gives finer control on stylization of convex and concave surfaces, although that also might make the modifier more technical and difficult to control.

Do you have any preference on this? To me both solutions look acceptable.

Well, that is good to know.

I guess that a normal user would expect a range from flat to maximally curved. This is the most straightforward approach, that I think I will use for now. Later/if there is demand we could implement the [-inf, inf] variant (like the subsurf modifier supports multiple subdivision types).

So I will make the function in utils.py return the negative Kr and use abs() in the shader.

updates in this revision:

  • fix the issue with negative Kr values
  • fix several bugs with some refactoring. The logic in the all the Crease Angle, and Curvature 3D thickness modifiers was flawed.

open issues from my side

  • I have not yet found a way to warn the user when a 3D Curvature shader is used with face smoothness off or a Crease Angle shader with face smoothness on.
  • asymmetrical thickness application is, codewise, not very pretty at the moment. It is also not currently exposed to the UI (should it be?).

Some help with these final issues would be appreciated. Furthermore I think that the code at least is now looking pretty good.

Experimental implementation of error messages for the user in Freestyle modifiers

In some cases, a Freestyle modifier won't function properly because of settings outside of the modifier. The best example is the Curvature 3D modifier from this patch. When the Face Smoothness option is off, the data that the Curvature 3D modifier uses is simply unavailable. The user should be notified of this fact.

The current solution is based on the error message in the Normal Edit modifier (When auto smooth is turned off) and looks like this:

A flaw with the current design is that the UI code has to be mixed with extra code checking a predicate and displaying a static error message. As far as I can see, there is no way around this, because the modifier that is drawn has no direct relation to the StrokeShader (modifier) that actually modifies the strokes. There is no way to communicate between the two.

So, I think that this implementation is the best one possible. As this is experimental, I've only implemented it for the color curvature and crease angle modifiers, but implementing for all the curvature and crease angle modifiers is straightforward. Feedback is welcome.

BTW, I am aware that the layout.separator doesn't work where I've used it.

As to warnings shown in the UI:

I was testing the same approach to show warnings about Curvature 3D modifiers used without the Face Smoothness option enabled. I think this is the way to go.

def draw_thickness_modifier(self, context, modifier):
    freestyle = context.scene.render.layers.active.freestyle_settings
    if modifier.expanded:
        ...
        if modifier.type == 'ALONG_STROKE':
            ...
        elif modifier.type == 'CURVATURE_3D':
            ...
            if not freestyle.use_smoothness:
                row = box.row(align=True)
                row.label(icon='ERROR', text="Warning: Face Smoothness option is off")

If I understand correctly, the Crease Angle modifiers may work even when the Face Smoothness option enabled. Of course lines may or may not appear on faces of the smooth shading depending on the option setting, whereas lines along edges of flat shaded faces should appear in the same way regardless of the Face Smoothness option. I believe we don't have to check the option within the UI of the Crease Angle modifiers.

Solving the two final problems

  • All three Curvature 3D modifiers now display a warning when Face Smoothness if off.
  • asymmetric thickness is now prettier codewise and available from the UI for the thickness noise modifier.

For the asymmetric thickness, I've verified that it works and saves correctly, but I'm not sure whether I've followed conventions/best practices.

A few functionality-wise comments are inlined.

release/scripts/freestyle/modules/parameter_editor.py
694

How about using a curve mapping to transform t into thickness rather than allowing only for simple linear interpolation?

699

Another word of choice would be "normalized" since the output value is in the range [0, 1] based on the given minimum and maximum values. Just my 2 cents.

757

The same comment as in line 694 applies here.

A few additional comments are inlined.

release/scripts/freestyle/modules/parameter_editor.py
635

Nature.CREASE is a bit mask, so that the last condition should be ... and (fe.nature & Nature.CREASE).

637

During a test on my side the acos() function failed due to a domain error (the argument is out of the valid range [-1, 1]) likely resulting from a numerical rounding error. Adding bound() would be the simplest solution: return acos(bound(-1.0, -fe.normal_left.dot(fe.normal_right), 1.0)).

Correcton:

release/scripts/freestyle/modules/parameter_editor.py
635

I meant or not (fe.nature & Nature.CREASE).

fixes after code review

  • renaming
  • use binary and for natures
  • implement a curvemapping for Curvature and Crease Angle thickness modifiers.

I did some further testing of the new modifiers. Here are some comments (inlined).

source/blender/blenkernel/intern/linestyle.c
336

Crease angle values are in the range [0, 180], so how about setting the default max_angle to 180 degrees?

422

The min_angle and max_angle props also need to be copied.

569

The same comment as in line 336 applies here as well.

834

The same comment as in line 336 applies here as well.

source/blender/freestyle/intern/python/BPy_FrsNoise.cpp
111

The parsing failure needs to be handled properly here. For instance giving a string as the argument should raise an exception because an integer number is expected.

source/blender/makesrna/intern/rna_linestyle.c
44

This line needs to be removed, since there is no color calligraphy modifier.

57

This line also need to be removed, because no alpha calligraphic modifier exists.

835

We don't really have a pattern in noise values, so how about saying "Change line color based on randon noise"?

Please, also consider updating the descriptions of the alpha and thickness noise modifiers.

870

In the description, I would say "... based on curvatures of 3D mesh surfaces".

Another thought: Would it make more sense to use "radial curvature" instead of just saying "curvature"? For surfaces, curvatures are evaluated with respect to a reference plane, and the curvature we refers to here is the radial curvature evaluated with respect to the plane defined by the surface normal at the evaluation point and the view point (camera position). This might appear too technical, but the proper name still gives some information to whom eventually look for it.

Anyway please consider changing related docstrings in the rest of the code.

876

The word "thickness" is likely a typo, right? Also, this description gives an impression that the modifier is applied conditionally up on a specific range of curvature values. That is not the case actually, so just saying "Minimum curvature" wound be fine.

881

The same considerations as in line 876 applies here as well.

1076

Most toggle switches in Blender has the "use_" prefix, so let us follow the same convention here. To me "use_asymmetric_thickness" seems a bit too long, so how about "use_asymmetric" for short?

inlined idea, patch with updates will follow shortly.

source/blender/makesrna/intern/rna_linestyle.c
1127

Most of the new shaders define their own thickness_min and thickness_max. the old ones use the value_min and value_max declared by rna_def_modifier_curve_common. Would it be an idea to create "rna_def_modifier_thickness_common" (or add an optional argument to rna_def_thickness_modifier) to declare a properly named thickness_min and thickness_max without all the code duplication?

updates after code review

This patch fixes all issues raised by code review.

I also extended the BoundedProperty type to support interpolation. saves complexity in the shaders and prevents bugs (silly swaps of variables and such).

an addition to my previous comment: The variable name 'value' is used internally (parameter_editor.py) for naming a thickness value. This should really be changed.

noticed a few errors in the new copy functions, will fix them in the next patch.

source/blender/blenkernel/intern/linestyle.c
423

I noticed this type, will fix it in a next commit.

933

min and max thickness are not copied.

950

curve doesn't get copied (actually causes a crash)

Folkert de Vries (flokkievids) updated this object.

Fix a corner-case in normal_at_I0D.

If a stroke has length 2, is_begin and is_end will both be True, which lead to a decrementation and a subsequent RuntimeError. This has now been fixed.

I've also updated the summary link to the manual for the new modifiers.