Differential D963
Freestyle: new stroke modifiers Authored by Folkert de Vries (flokkievids) on Dec 31 2014, 5:25 PM.
Details 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:
The documentation for these new modifier types can be found in the manual:  (left: AnisotropicThicknessShader, right: NoiseThicknessShader)  (left: Curvature 3D, right: Simplification)
Diff Detail Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions 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:
Comment Actions Still trying to tame my new linux setup... I missed several whitespace issues and fixed some sorting and spelling here and there. Comment Actions Hi flokkievids, Please, find below my comments on the latest version of Patch D963 (new stroke modifiers).
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.
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 would prefer the word tangent, since the Tangent shaders work on a different quantity than the curvature as mentioned above.
I agree and we can add an option to modify thickness in an asymmetric way.
Thanks for the problem report. The missing call of StrokeShader.init(self) should be properly detected and a crash should not happen.
To me the present function names are reasonably good and acceptable.
Comment Actions Corrections (inlined).
Comment Actions some questions (inlined). A first set of changes should be ready soon.
Comment Actions some changes
Comment Actions 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.
Comment Actions A small question based on your comment
Comment Actions 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: Comment Actions more changes after code review
Comment Actions 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. Comment Actions 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. Comment Actions updates in this revision:
open issues from my side
Some help with these final issues would be appreciated. Furthermore I think that the code at least is now looking pretty good. Comment Actions 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. Comment Actions 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. Comment Actions Solving the two final problems
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. Comment Actions A few functionality-wise comments are inlined.
Comment Actions A few additional comments are inlined.
Comment Actions Correcton:
Comment Actions fixes after code review
Comment Actions I did some further testing of the new modifiers. Here are some comments (inlined).
Comment Actions inlined idea, patch with updates will follow shortly.
Comment Actions 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. Comment Actions 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. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||