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.

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.

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.