Page MenuHome

Improvements to the cpp-defined Freestyle stroke shading
Needs ReviewPublic

Authored by Folkert de Vries (flokkievids) on Aug 17 2014, 12:16 AM.

Details

Summary

I've used the past week to update some of the cpp-defined stroke shaders. I've used imagemagick and some python scripting to check for errors, but if I, with my limited c/cpp knowledge, have done something stupid please tell me.

changes:

  • use StrokeVertex.u instead of (index of vertex / number of vertices). it is a lot cleaner and actually more correct. this causes slight deviations in render results, but they are barely noticeable.
  • I tried to use float arrays and blender's vector operations where it makes sense (that is, where it's shorter and more readable)
  • implemented the .orthogonal() method for vec2d objects; much cleaner and usually saves a line.
  • tried to improve sampling a bit. memory-wise it should be a little more efficient, speed-wise it barely makes a difference. the result is exactly the same.
  • fixed the guiding lines-shader, taking the python version as the right one. the cpp version would not displace some
  • strokes and in general showed some weird behavior. there is however quite the visual difference between the two versions.
  • using a new deletion method for removing consequtive parts of a stroke, the BezierShader is now faster, especially on dense strokes (600% with sampling 0.1). the same is now true for the tipremovershader, which for dense strokes (sampling 0.1) is now 10% faster. vertex deletion is a real bottleneck. (obviously a sampling value of 0.1 is somewhat impractical, but it shows this flaw quite strikingly.)
  • removed lots of unnecessary code. this produces a 1-3% speed increase, but more importantly much cleaner code.
  • (some more ifdef'd code could also be removed in my opinion)
  • I tried to come up with an alternative method for the LengthDependingThicknessShader. it seems to work quite well already, and is much more predictable that the old version.
  • I would furthermore propose (perhaps after a consultation to the blender-npr mailing list) to expose the smoothingshader and the thicknessnoiseshader to the UI. they are sort of on a shelf catching dust, and I could see them being quite useful.

minor points I found out later:
qt messed up the indentation, will fix that next patch
there is a slight error in tipremovershader

Diff Detail

Event Timeline

Folkert de Vries (flokkievids) retitled this revision from to Improvements to the cpp-defined Freestyle stroke shading.
Folkert de Vries (flokkievids) updated this object.
Folkert de Vries (flokkievids) updated this object.

Thanks a lot for the code clean-up and optimization efforts. I went through the revisions quickly and made some notes. I would appreciate it if the indentation and white space changes are fixed, so the code review will be easier. In the next iteration I will check the code changes in more detail.

source/blender/freestyle/intern/stroke/BasicStrokeShaders.cpp
747

I guess the condition has to be nExtraVerts <= 0.

source/blender/freestyle/intern/stroke/Stroke.cpp
679

_Vertices.erase(it1, it2) does not call delete for each of elements to be removed. Iteration from it1 to it2 is necessary to delete the elements.

693

Stroke::RemoveVertex() does not call UpdateLength() so we could omit this call here. Updating the stroke length is costly. If RemoveVertexRange() is called multiple times in a loop, then all but the last update is redundant and less efficient.

Folkert de Vries (flokkievids) edited edge metadata.

updates:

  • whitespace fixes
  • proper deletion of removed StrokeVertex objects
  • rewriting of vector operations (could often be compacted)

further questions

  • what's your opinion on exposing some of the currently existing but unused cpp-defined shaders to python/the parameter editor
  • what is the consensus on the use of vector objects? (float array vs freestyle vectors). To me the freestyle vectors seem the right tool for the job in many cases, but are sometimes unnecessarily bulky. For some cases using float arrays looks like the best (shortest, cleanest) option.
  • There is a lot of code that is currently unused and of which I'm quite sure it never will be. (ThicknessVariationPatternShader, ColorNoiseShader, BSplineShader). Maybe now would be a good moment to get rid of some of those (if they are ever needed again, they are in GIT)?

Just to reply to the questions:

what's your opinion on exposing some of the currently existing but unused cpp-defined shaders to python/the parameter editor

Exposing more shaders is welcome from both users and developers perspectives. That is really a matter of additional coding efforts (preferably in the BCon1 or BCon2 phase).

what is the consensus on the use of vector objects? (float array vs freestyle vectors). To me the freestyle vectors seem the right tool for the job in many cases, but are sometimes unnecessarily bulky. For some cases using float arrays looks like the best (shortest, cleanest) option.

There is no much consensus there, since a very few developers have been working on the code so far (:

I have been mostly sticking to Freestyle vectors because of easier integration to the existing code base. Alex, who contributed the optimization patch for view map construction, also used Freestyle vectors only.

Freestyle vectors are C++ classes so they benefit from C++ features. For instance methods of Interface0D and Interface1D subclasses return references to vector instances. Unless we need to store the values for subsequent modifications, we want to keep them as references to avoid unnecessary copying of the values (since making vector copies can be a source of performance loss). Float arrays have smaller memory footprints than C++ vectors of the same precision and size, at the cost of keeping their size separately (by hard-coding the size or storing it somewhere). Operation-wise both do the same computations.

If differences between the two do not matter (which depends on the situation), then I don't mind if you prefer one to the other.

There is a lot of code that is currently unused and of which I'm quite sure it never will be. (ThicknessVariationPatternShader, ColorNoiseShader, BSplineShader). Maybe now would be a good moment to get rid of some of those (if they are ever needed again, they are in GIT)?

Agreed, and I think we can remove unused lines of code for better code maintainability.

Here are a few more comments on the code changes.

source/blender/freestyle/intern/stroke/BasicStrokeShaders.cpp
114

Please, take care of indentation in continued lines here and in the rest of the patch. Tab and space have to be carefully combined according to the Blender coding style. Bastien once cleaned up the entire Freestyle code base in that level of detail, so we need to adhere the coding style not to make Bastien work on clean-ups again! (:

119

Good idea to use interpf() here!

By the way, please remove the space between v and ->u() here and in the rest of the patch.

139

I agree that having this special condition does not make much sense. I think we can remove it.

some updates:

I switched back to Freestyle vectors for the most part. The whole API is built upon them; using float arrays seems out of place and requires a lot of conversion code.

For clearer Vector code, I introduced some new vector functions: to_2d(), to_3d() and negate(). Additionally, a method is added for setting all color components + alpha with a single call. I think it's much cleaner and also minimally faster (measured from python).

We spoke about the removal of some old/unused code before. I propose removal for:

  • ThicknessVariationPatternShader
  • ColorVariationPatternShader
  • ColorNoiseShader
  • BSplineShader
  • Inflateshader (I quickly ported this one to python. results seem weird (not so useful)
  • Omissionshader (already ifdef'd out)
  • streamShader
  • fstreamShader

I'm somewhat doubtful about:

  • CalligraphicColorShader
  • MaterialColorShader

for this removal, I think it would make sense to do it in a separate patch.

one further question: why is StrokeVertex.Point2d a 3D vector? I know there is .getPoint() which does return a 2D vector object, but I think the name Point2d is really confusing.

Here are replies to selected comments:

I switched back to Freestyle vectors for the most part. The whole API is built upon them; using float arrays seems out of place and requires a lot of conversion code.

Sounds good.

For clearer Vector code, I introduced some new vector functions: to_2d(), to_3d() and negate(). Additionally, a method is added for setting all color components + alpha with a single call. I think it's much cleaner and also minimally faster (measured from python).

These additions are appropriate and very welcome.

We spoke about the removal of some old/unused code before. I propose removal for:

  • ThicknessVariationPatternShader
  • ColorVariationPatternShader
  • ColorNoiseShader
  • BSplineShader
  • Inflateshader (I quickly ported this one to python. results seem weird (not so useful)
  • Omissionshader (already ifdef'd out)
  • streamShader
  • fstreamShader

I agree with the removal of all the above shaders.

As a side note, the description of the Inflateshader (quoted below) is quite straightforward and easy to understand.

/* Shader to inflate the curves. It keeps the extreme points positions and moves the other ones along the 2D normal.
 * The displacement value is proportional to the 2d curvature at the considered point (the higher the curvature,
 * the smaller the displacement) and to a value specified by the user.
 */

For curiosity I also tried to port the shader to Python and with a slight modification I got a result like below which makes sense.

In complex input scenes, this shader is likely to produce weird results. So, yes, I agree not to keep it in the code base.

I'm somewhat doubtful about:

  • CalligraphicColorShader
  • MaterialColorShader

    for this removal, I think it would make sense to do it in a separate patch.

Agreed.

one further question: why is StrokeVertex.Point2d a 3D vector? I know there is .getPoint() which does return a 2D vector object, but I think the name Point2d is really confusing.

The 3rd component of StrokeVertex.getPoint2D() gives the normalized Z depth of the vertex in the camera coordinate system.

The reason why StrokeVertex has .getPoint() in addition to .getPoint2D() is that the 2D coordinates of a StrokeVertex can be modified.

All view map components except for StrokeVertex are static, so that vertex coordinates of 0D elements such as SVertex and CurvePoint cannot be modified from within style modules. The .getPoint2D() method gives access to these static vertex coordinate values.

Strokes on the other hand allow modifications of geometry and other attributes through user defined stroke shaders. The .getPoint() method refers to the mutable 2D vertex coordinates.

All 0D elements have a .getPoint2D(). In addition, StrokeVertex has .getPoint() because its coordinates are mutable.

I agree that the two names appear confusing. Any renaming proposal if you have is welcome.

thanks for the comments

I've attached a patch that removes the shaders.

regarding the point2d: is that third component actually used somewhere (couldn't find it but guess so) where it makes sense to get both the coordinates and the normalized z-depth in one object. It might make more sense to expose the z-depth separately and let Point2D be of type Vec2d.

The patch for removing unused shaders is much appreciated. Could you please create a new differential revision from the patch? StrokeTextureStepShader should not be removed.

StrokeVertex::getPoint2D() is mostly necessary in the C++ layer. All 0D elements are subclasses of Interface0D, and many C++ API components take an Interface0D object to apply operations on any 0D element in a generic way. Having the normalized Z depth as the 3rd component of a vector object may come in handy, since vertex operations (e.g., interpolation) are likely to involve the X and Y components as well as the Z depth so as to keep all of them consistent.

  • added scalar/vector operations (in line with blender's internal vector library)
  • general improvements to the creation and usage of containers
  • general cleanup

I think this patch is pretty much finished now, as far as I'm concerned. Review would me much appreciated.