Page MenuHome

Improvements to the Freestyle Python API (needed by the SVG Exporter)
ClosedPublic

Authored by Folkert de Vries (flokkievids) on Apr 18 2015, 2:34 PM.

Details

Summary

This patch adds some new functionality to the Freestyle Python API, notably:

  • MaterialBP1D, checks whether the supplied arguments have the same material
  • Fixes a potential crash in CurvePoint.fedge (due to NULL pointer)
  • Makes (error handling in) boolean predicates more robust
  • Adds a BoundingBox type, to make working with bounding boxes easier
  • Adds several new functions (get_object_name, get_strokes, is_poly_clockwise, material_from_fedge)
  • Adds a StrokeCollector StrokeShader, that collects all the strokes from a specific call to Operators.create()
  • Adds hashing and rich comparison to the FrsMaterial type

These new features (most of them, anyway) are needed for making a more robust SVG exporter that supports holes in fills.

Diff Detail

Repository
rB Blender

Event Timeline

Folkert de Vries (flokkievids) retitled this revision from to Improvements to the Freestyle Python API (needed by the SVG Exporter).
Folkert de Vries (flokkievids) updated this object.
Folkert de Vries (flokkievids) set the repository for this revision to rB Blender.

Here are comments for the first round of code review. The patch looks acceptable quite soon.

release/scripts/freestyle/modules/freestyle/predicates.py
242

The check for one or more predicates has been omitted. Is that intentional?

255

The same comment on line 242 is applicable here as well.

574

The same comment on line 242 is applicable here as well.

587

The same comment on line 242 is applicable here as well.

675

This predicate actually compares diffuse colors rather than entire materials. Would it make more sense to rename the predicate to something like SameDiffuseColorBP1D?

Another thought: Node-based diffuse colors in Cycles are not available as freestyle.types.Material.diffuse. For this reason the Material.line color property has been added (see the "Line color properties for BI and Cycles" section of the Blender 2.73 release notes). Users might want to have a predicate that works in the same way in the Blender Internal and Cycles.

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

Just a thought: Is there a preferred use of quotation marks for docstrings? We have triple double-quotes, single double-quotes, and single single-quotes in this file. How about using one of these styles throughout the module?

Thanks for the quick review, I've inlined my responses.

I will wait with an updated version till we find a solution for MaterialBP1D.

release/scripts/freestyle/modules/freestyle/predicates.py
242

You are right, even though

all() returns True when the argument is an empty sequence (so that is a problem here)
any() returns False when the argument is an empty sequence

I will add a validity check in the if-statement ("explicit is better than implicit")

675

Well,

The materials of two viewedges can have the exact same colors, they are still different objects with different memory locations and a.eq(b) will return False. Unless this can be fixed (it seems a waste of memory, really), comparing the Material objects doesn't work (nor does hashing them in a set like I've done here).

The goal of this predicate is to check whether the two ViewEdges have the same material (appearance), to correctly chain two separate objects that can then be interpreted as a single shape. I don't think that the line color is relevant in this context.

Finally, when using a build without cycles (which I do for faster rebuilds), the Freestyle line color is always (0, 0, 0, 0) and not visible in the UI. Based on the description on the wiki, I think that is a bug.

So the options are to make Material objects with the same properties equal (==) each other, or to say that a test based on the diffuse color is enough for Freestyle's purposes.

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

the bpy.extras module seems to use triple double-qoutes for all docstrings and this is (as far as I've seen) the general convention. I will go over the docstrings and correct the other qoute types.

@Tamito Kajiyama (kjym3)

I've done a little investigating.

I think that caching FrsMaterial objects with shapes having a material pointer to an object in the cache is technically possible, but it may not be the most straightforward thing. Every material assignment will then have to be checked against the cache (which will need a good __eq__/operator= anyway), really it will make things more complex. As you have more experience with the code base and C++ I'd like to hear your opinion though.

An alternative route (albeit using more memory) would be to implement a good eq (and maybe hash) function on BPy_Material objects. I really like the use of sets in my current implementation of the MaterialBP1D, but if a hash is not feasible then I can rewrite to use eq.

let me know what you think.

I do think we can define the equality of two FrsMaterial objects in one way or another. That's not really a big deal.

An issue I was trying to highlight is that FrsMaterial does not support node-based Cycles materials at all.

The FrsMaterial class is intended to store a small subset of properties available in Blender's Material ID data blocks. All FrsMaterial properties (i.e., diffuse, specular, and so on) are static in the sense that they are represented by RGBA values and can be determined before rendering is performed. The API design of the FrsMaterial class is just fine with this type of static material properties used by the Blender Internal.

Node-based Cycles materials on the other hand are defined in terms of many factors including mesh geometry, lighting, camera settings and so forth. Colors are computed on a per-pixel basis at the rendering time. FrsMaterial is simply incapable of this type of dynamic color properties used by Cycles.

Given this constraint, we have several approaches.

One is to leave the node-based materials issue open for now and focus on static color properties. This makes the comparison of FrsMaterial objects trivial, i.e., we just need to compare individual color properties one after another in an element-wise fashion (that has already been implemented somewhere in the code base).

Another option is to extend FrsMaterial to store pointers to Cycles material nodes (they are ID data blocks) and compare the pointers when equality is checked. Note that this is a comparison of references and not of values, whereas static color properties are compared by values.

Yet another option is to compare material identities by reference in all cases including Blender Internal materials as well. To this end we might want to define another material wrapper class aside from FrsMaterial.

For the latter two approaches, we need to extend the BlenderFileLoader class.

One is to leave the node-based materials issue open for now and focus on static color properties. This makes the comparison of FrsMaterial objects trivial, i.e., we just need to compare individual color properties one after another in an element-wise fashion (that has already been implemented somewhere in the code base).

Take a look at FrsMaterial::operator=(const FrsMaterial&) in source/blender/freestyle/intern/scene_graph/FrsMaterial.h. The Python FrsMaterial wrapper needs a rich comparison operator based on this method.

Campbell Barton (campbellbarton) added inline comments.
release/scripts/freestyle/modules/freestyle/utils.py
139

In Python3 theres no need to subclass object.

Folkert de Vries (flokkievids) edited edge metadata.

Revisions after code review

  • removed subclassing of object
  • implemented richcmpr for FrsMaterial
  • made use of qoutes consistent in utils.py
  • added an extra check to the boolean predicates, to verify correct input

@Tamito Kajiyama (kjym3)

Could you maybe explain your concerns with the MaterialBP1D with respect to cycles again? I can't get cycles to build atm, so I'm not sure there even is a problem, but judging by your earlier message I guess that there is. Was this solved with the richcmpr or are other solutions still needed? If so, how can we tackle this problem?

My concern was two-fold. One was that materials for Cycles have node-based colors and static colors. The latter is shared by Cycles and the Blender Internal and employed only when nodes are not used. FrsMaterial can access static color properties only. Another issue was that MaterialBP1D was comparing only diffuse colors. This latter problem has already been addressed by the rich comparison operator exposed to the Python wrapper of FrsMaterial.

In the screen capture above, the left and right half shows a material with nodes disabled and enabled, respectively. The diffuse color on the left is a static color property, whereas the diffuse color on the right is a node-based color. The same word "diffuse color" here refers to two different data elements. Among the two, FrsMaterial can retrieve only the static diffuse color. This is still an open issue (and I believe it is difficult to address).

Additional remark: The Freestyle Line color property (seen in the screen capture) is an exception in this discussion. The Freestyle Line color refers to the same data element with and without nodes in Cycles as well as in the Blender Internal.

I hope this explains my concern better.

As I understand your comment, there is a general issue with FrsMaterial, that is not specific to the SVG exporter/this patch. It was earlier, when MaterialBP1D compared diffuse colors, but now that it uses richcmpr, this problem has been solved. The remaining issue is therefore not (specifically) relevant to this patch. is that correct?

On a side note: I've started work on adding a page about SVG exporting to the Blender manual. Once this patch is in master, I'll update the addon and try to get this piece of documentation accepted.

Yes that understanding is correct. And the SVG addon page is nice to have.

Are there any open issues here still? I'd like to get these changes in before the next Blender release. If that isn't possible due to time constraints, I'd like to at least get the C/C++ parts in (all Python stuff can come with the exporter itself for now).

@Folkert de Vries (flokkievids),
I think the patch is acceptable, and can be merged as it is. My only concern is that we are now in strict BCon3 (no new features allowed). This patch partly aims at fixing crashes, while it contains new stuff.

Campbell,
Could you please take a look at the patch from a code reviewer's perspective as well as with regard to whether the patch can go into the trunk now?

@Tamito Kajiyama (kjym3)

That is exactly why I asked. I would be okay with splitting the patch up, but would like at least the crash fix, improvements to the binary predicates and the MaterialBP1D predicate in trunk now (so that I can make an update to the SVG Exporter). From my point of view, these could be classified as bug fixes, except the actual MaterialBP1D declaration, but that is 12 lines of code that stands completely on its own.

found a large error (no idea how this hasn't been spotted yet: fills just don't work anymore due to this change. I will update the patch.

source/blender/freestyle/intern/python/BPy_FrsMaterial.cpp
503

There is a big flaw here: the == operator used is not FrsMaterial's operator== (but instead just a comparison on the pointer value). This current implementation gives unexpected results.

small but important fixes

  • make FrsMaterial's richcmpr compare Material values instead of memory addresses.
  • BoundingBox now only accepts 2D vectors, the 3D extentions added a lot of extra code, it is not needed and there seemed to be a bug in the logic as well.
  • some other small things

@Folkert de Vries (flokkievids),
Could you please confirm that the planned update to the SVG Exporter you mentioned is considered a bug fix?

That is correct. The SVG exporter currently gives unexpected results when exporting complex shapes (specifically shapes with holes). The changes in this patch (specifically the three aspects I mentioned in my previous comment) are needed to fix this issue.

Minor edits, only pep8/formatting

  • Missed last commit
  • Use slots for BoundingBox class

Is the updated SVG export addon available anywhere?

Made some minor changes, but wasn't able to test.

Note on material hashing.

release/scripts/freestyle/modules/freestyle/predicates.py
679–683

Why not add a hash function here? (see how existing code assigns tp_hash = (hashfunc)_Py_HashPointer;)

Though in this case you'll need to wrap it...

static Py_hash_t FrsMaterial_hash(PyObject *self)
{
	return _Py_HashPointer(((BPy_FrsMaterial *)self)->m);
}

Then you can do...

materials = {material_from_fedge(fe) for fe in fedges}
return len(materials) < 2

@Campbell Barton (campbellbarton),
Many thanks for the prompt code review!

@Folkert de Vries (flokkievids),
I believe the material hashing Campbell suggested works, since BlenderFileLoader::insertShapeNode() adds new FrsMaterial objects only when materials are different when compared by value and regardless of objects the materials belong to. Could you please test the idea and check if the SVG exporter issue can be addressed?

Thanks for the activity here, I'll do a thorough test this afternoon. Posting a reaction now to be more efficient with time zones.

regarding the hash function: I was under the impression that a new material (with a new memory address) is created for every vertex. That (I think, didn't yet test this) would mean that the suggested hash method wouldn't work.

The new SVG exporter can be found here
(there are some rudiments of bpy.context in the import logic. this is irrelevant to the patch and should have no effect on the output)

A testfile:

important are:
layer 1: the holes in both objects should be rendered correctly in the SVG
layer 4: the order, filling and color of all four planes should be correct.

I've done my tests:

The current version of the patch produces the expected result and doesn't give any errors. All seems good to me.

The proposed method of hashing doesn't work. As I thought, all the materials, even though they are the same values, are different objects (different memory address, therefore different pointer, therefore a different hash).

There are, I guess, three options:

  • leave it as it is; the current code is not idiomatic, but quite clear
  • make sure that equal (by value) materials are equal by reference (so the same object in memory)
  • make a by-value hash

The first option seems by far the easiest, although the excessive memory allocation for materials may be something that should be solved anyway.

Option 1) is fine for release I suppose.
Option 2) it seems strange materials are being duplicated all over.
... Avoiding this seems reasonable long term?
Option 3) Could in fact be done quite easily since its doing a byte-for-byte comparison - hashing the data can work.
Could make its own hash function or just hash the data its self with BLI_hash_mm2.

the mm2 hash indeed works, the implementation is quite simple

#include "../../blenlib/BLI_hash_mm2a.h"

static Py_hash_t FrsMaterial_hash(PyObject *self)
{
	return (Py_uhash_t)BLI_hash_mm2((const unsigned char *)self, sizeof(self), 0);
}

i've verified that this works for the set creation, so to me this looks like the best solution at the moment.

@Tamito Kajiyama (kjym3) what is your opinion on the wasteful creation of FrsMaterial objects? Also, wouldn't a hashing method be useful for other kinds of objects (StrokeVertex, StrokeAttribute)?

The use of hash looks great. I agree with Campbell's suggestion to avoid duplicates of the same FrsMaterial object by value, although I would leave it for future work and not address it here in this patch.

@Tamito Kajiyama (kjym3)
Agreed. It doesn't have a high priority right now but should be fixed. I guess that StrokeAttribute has similar issues, so we may need to look at a more general solution.

@Campbell Barton (campbellbarton)
Is this patch (with inclusion of the hash function + set comprehension) OK for master now, or are there further issues?

@Folkert de Vries (flokkievids). the patch /w hashing is fine AFAICS.

note, in your example above...return (Py_uhash_t)BLI_hash_mm2((const unsigned char *)self, sizeof(self), 0);

should be sizeof(*self)

Folkert de Vries (flokkievids) updated this object.
Folkert de Vries (flokkievids) removed rB Blender as the repository for this revision.

add hash function for the FrsMaterial type + set comprehension in MaterialBP1D.

Some minor comments (inlined).

source/blender/freestyle/intern/python/BPy_FrsMaterial.cpp
518

It should be okay to do #include "BLI_hash_mm2a.h" without the relative path included. Could you also move it to the beginning of the file?

522

Please double check if the size parameter should be sizeof(BPy_FrsMaterial), since sizeof(*self) is equivalent to sizeof(PyObject) and hence it is smaller than the size I suggest.

Thanks for the comments

The header file is now at the top of the file. The compiler is only happy when the #include is within the extern "C" block, so that is where I've put it.

regarding the hash: The hash is created from a PyObject, and therefore the size of a PyObject is used. Using sizeof(BPy_FrsMaterial) for creating a hash from a PyObject gives incorrect results. Maybe it would work if the PyObject is cast to BPy_FrsMaterial, but that seems unnecessary.

This revision was automatically updated to reflect the committed changes.