Page MenuHome

Freestyle: Improve FrsMaterial
Needs ReviewPublic

Authored by Folkert de Vries (flokkievids) on Jul 18 2015, 6:09 PM.

Details

Summary

This patch aims to solve a problem with the ViewMap cache and FrsMaterial. currently, FrsMaterial copies properties from Material and caches those. Changes to (for instance) diffuse color after the cache has been made won't show up in a render. This is obviously a problem.

Diff Detail

Event Timeline

Folkert de Vries (flokkievids) retitled this revision from to Freestyle: Improve FrsMaterial .Jul 18 2015, 6:09 PM
Folkert de Vries (flokkievids) updated this object.
Folkert de Vries (flokkievids) updated this revision to Diff 4699.

This is a very rough first version of FrsMaterial as a proxy for Material. I made this to see what tricky parts of the implementation are.

A major issue (see various inlined comments) is that FrsMaterial and Material don't align very well in terms of properties. Color's don't have alpha values, emission is a float instead of a float[4], ect.
I'm not quite sure how the backwards compatibility works, but I'd like to remove as much useless stuff as possible.

Furthermore, Material doesn't store specular and diffuse as float arrays. returning them as an array of pointers to float changes the return type of certain functions. Not sure whether this is a problem.

Finally, what should happen if the reference to Material is invalidated.

warning: This compiles, but there may be very stupid/naive stuff going on. I haven't yet ran this code.

source/blender/freestyle/intern/scene_graph/FrsMaterial.h
82

Material doesn't store colors in float arrays, but rather as separate values. The approach using Diffuse (line 47) seems to work, but won't have the same return type. This needs some work.

108

Material's colors don't have a separate alpha value. could use _Material->alpha, just 0 or remove it completely.

172

Material only has a single emission value. It is not a color. Because Emission is never used in relation to Blender Materials, I think it should be removed.

298

small error, won't matter anyway.

Do you think it is easier to remove FrsMaterial completely and instead use Material?

IMHO we can break backward compatibility if we don't break any existing .blend files that rely on the Parameter Editor.

removing FrsMaterial completely is an interesting idea, but I don't really see a compelling reason for it at the moment.

FrsMaterial is used 121 times through the codebase and replacing it with Material is non-trivial. The change would require functions (not methods, methods would be a lot nicer) to extract the various properties as (cpp-defined Freestyle) vectors. I also don't see what immediate problem it would solve. On the other hand, the current FrsMaterial object is clearly not the most ideal representation of a material in the Blender context.

setDiffuse is used 14 times in the codebase. By my estimation, a quarter is not used anymore and half of them are setting a material to a static list of default values. There are some other problems, for instance Specular having an alpha value doesn't make sense (in Blender), so I'd like to remove this field. Some quick code searches show that these methods are used <10 times, so that looks do-able. This seems to also be the case for the other unneeded FrsMaterial fields.

So, I think that it is best to rewrite FrsMaterial as a read-only interface to Material, while removing all unneeded fields/attributes. This allows us to implement custom methods to this type to make the integration with the rest of the Freestyle codebase easier.

I would also propose to remove (not just #if out) code that uses the deprecated FrsMaterial but isn't used anymore (ViewMapIO is the only place I've found so far, but I'm quite sure there will be more).

Finally, how will/could we break backwards compatability by changing something that only realy exists at runtime? As far as I can see, FrsMaterial never gets saved to disk, so how would this be a problem?

So, I think that it is best to rewrite FrsMaterial as a read-only interface to Material, while removing all unneeded fields/attributes. This allows us to implement custom methods to this type to make the integration with the rest of the Freestyle codebase easier.

I agree with you. It seems the best way to go that we make (backward incompatible) API changes in FrsMaterial so that it provides a better interface and straightforward mapping to Material.

I would also propose to remove (not just #if out) code that uses the deprecated FrsMaterial but isn't used anymore (ViewMapIO is the only place I've found so far, but I'm quite sure there will be more).

That also sounds fine to me.

Finally, how will/could we break backwards compatability by changing something that only realy exists at runtime? As far as I can see, FrsMaterial never gets saved to disk, so how would this be a problem?

The C++ FrsMaterial class has a Python wrapper which is used in style modules (including the Parameter Editor) through functions of the UnaryFunction0DMaterial type and material-related shaders. API changes in FrsMaterial hence will break existing style modules.

That is the route we'll take then. I also see the incompatibility issue now, although I assume that it'll be quite limited, because there are very few uses of FrsMaterial (and its function type) in the Python API and the colors are wrapped in vectors, so for instance removing alpha may not even cause a problem in most cases.

A c/c++ noobie question: how can we elegantly deal with the free'ing of a Material that an FrsMaterial points to? (elegantly as in not checking for NULL in every function).

I also think that we should store a default Material (this should already exist, as even material-less objects render with a color + shading) and associated FrsMaterial, to fall back on.

I believe we can assume that a Material object always exists and never gets freed while a wrapper FrsMaterial for it exists. As we discussed FrsMaterial objects will be created on the fly when underlying Material objects needs to be exposed to style modules in Python during the Freestyle line rendering process. During Freestyle rendering, Material objects won't be freed. By the same token, we don't really need a default material for FrsMaterial.

Folkert de Vries (flokkievids) updated this revision to Diff 4826.

Freestyle now works with FrsMaterial_

This patch compiles and hasn't crashed so far with some simple tests. The problem with the viewmap cache is solved.

@Tamito Kajiyama (kjym3), if you're okay with these changes, I'll remove FrsMaterial and rename FrsMaterial_.

EDIT:

using a material modifier with the settings:

alpha -> crashes. alpha is now separate from diffuse, but python is unaware of this.

specular hardness -> gives a python error. this was never implemented in python, now may be the time.

The rest of the options seems to work. Material functions still need testing.

some tidying up

  • adding specular (was always black previously)
  • implement alpha
  • implement operator== & !=
  • fix shininess/hardness and alpha in python

some inline questions and notes

main open topics:
can Emit and Ambient be completely removed?
can Specular and Diffuse have length 3 instead of 4
what do we do with ViewMapIO? it's included, but I can't really figure out whether it does anything useful. I've now ifdef'd large parts of it, but if it's useless, I'd rather see it removed.

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

which one should it be?

source/blender/freestyle/intern/scene_graph/FrsMaterial.h
48

do these still need to be float[4], I would rather see float[3]

50

Emit is already commented out, but Ambient is not used by Python either (Material modifier, that is). can it be removed completely?

extra argument for this is that in python the 'embedded' Material is available, so any of its properties are.

98

missed this one

208

do we use shininess or hardness?

306

I learned something new about c/c++ interaction after diff'ing, and the this commented line is now the implementation. (idem for operator==)

lesson:
operator!= is not implemented automatically for a C-defined type, while operator== is.

320

should _Material be settable.

I think FrsMaterial as completely immutable would be better.

Hi flokkievids,

The patch works fine functionality-wise from within the Parameter Editor, so the reported regression related to view map caching is considered fixed. The only concern I have is thus the implementation of the new FrsMaterial_ class (which could eventually replace the old FrsMaterial class).

I believe the FrsMaterial_ implementation could be much simplified by taking account of the fact that the storage of the diffuse RGB values (Material::r and so on) is contiguous and hence &r can be used as if it is a pointer to an array of 3 float elements. More detail is provided through inline comments below.

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

SPEC_HARD is the defined symbol as in source/blender/makesrna/intern/rna_linestyle.c.

source/blender/freestyle/intern/scene_graph/FrsMaterial.h
50

My understanding is that the concepts of ambient and emit colors come from OpenGL on which the stand-alone version of Freestyle was heavily depending. In the context of Blender these color properties do not make much sense (even Freestyle::BlenderFileLoader did not use them at all). Hence both ambient and emit can be completely removed IMHO.

82

The storage of the diffuse RGB values in Material is guaranteed to be contiguous because the struct fields are defined as:

float r, g, b;

This means that &r refers to the address of the first of the three float values and hence can be used as if it is a pointer to an array of 3 float elements. This is a common practice in C/C++.

97

This method could be defined as follows:

inline const float* const diffuse() const { return &_Material->r; }

And hence FrsMaterial_::Diffuse can be removed. This definition is consistent with the original code before the patch. I don't see any reason to change the types of FrsMaterial_::Diffuse from an array of float values to an array of float pointers. The same comment applies to specular as well.

208

I believe shininess is also a residual of Freestyle dependency on OpenGL and can be removed. From Blender users' perspective it does not make much sense.

Specular hardness seems well defined in Blender so we could keep it in FrsMaterial_ IMHO.

Replace FrsMaterial

The new version of FrsMaterial is now the only version. So far, it seems without problems (no crashes or visible regressions)

But...

Although updating a material now works even if the ViewMap is cached, adding a new material or changing the order of materials has no effect. The solution would be to make VlakRen->mat a (pointer to the topmost material of object x) instead of (pointer to the current topmost material of object x). VlakRen is outside of the Freestyle context, so this may have unforseen consequences and I have no idea whether it is possible at all.

Some inline comments.

I read your message via email, so I didn't notice the inline comments. I'll have a look at them shortly.

release/scripts/addons
2

disregard this. I have my addons as a separate repository that is symlinked. the main blender repo doesn't like this. Ideas to fix this are welcome.

source/blender/freestyle/intern/blender_interface/BlenderFileLoader.cpp
584

I added an assertion here. All vlakken (vlak, plural vlakken, in blender has a dutch origin, I believe) should have a material. If they don't, something higher up the chain is wrong.

585

to store Material and FrsMaterial, currently, two vectors are used.

I would prefer to use std::map<Material *, FrsMaterial *>. This datatype has a find method and seems to me the 'right tool for the job'.
The reason why I didn't do this already is that the LoaderState needs a current material index. An std::map is ordered by keys (so not by insertion order), so an index would be meaningless. It seems though that this currentMIndex is never read, only written. would it be possible to make currentMindex a key value (that is, Material*)?

Furthermore, the order of the FrsMaterials doesn't really seem to matter, so maybe currentMindex can be completely removed?

updates:

  • creation of a FrsMaterial from python now works
  • renamed Material (in a python/freestyle context) to FrsMaterial
  • removed Ambient, shininess and Emitting
  • removed Diffuse and Specular
  • use 'SPEC_HARD' in python

This is comming along quite nicely. I got 'importing' a Python RNA object (material) into C working, but the other way around (Material* to PyObject*) doesn't yet work correctly (gives a segfault). Some help here would be appreciated.

Folkert de Vries (flokkievids) marked 13 inline comments as done.Aug 14 2015, 2:24 PM

Cleaning up the inline comment noise.

Open issues:

  • use of std::map to store (Material *, FrsMaterial *) pairs.
  • what to do when the material order is changed/a material is removed while the viewmap is cached
  • limiting copying/duplication of FrsMaterial values.
  • returning a Material* to python
source/blender/freestyle/intern/scene_graph/FrsMaterial.h
82

That works nicely.

Still, the size is not encoded in the type, though we have in fact changed it (from float[4] to float[3]). I have updated uses of diffuse and specular accordingly.

161

This copy constructor is called very often, In particular from BPy_FrsMaterial_from_FrsMaterial in BPy_Convert.cpp. Can we limit that? I tried to (by re-using a pointer to FrsMaterial for many BPy_FrsMaterial objects) but that gave a segfault.

There are a few more locations where FrsMaterial is copied where it (in my novice opinion) doesn't make sense any more.

Figured out the Python -> C -> Python conversion for StructRNA objects. There may still be a better way to do this, but this works.

a minor annoyance is the need for const_cast in FrsMaterial_material_get. I think it is harmless, but would like some advice here.

Thanks for the patch updates. A few inline notes are found below.

source/blender/freestyle/intern/scene_graph/FrsMaterial.h
99

This method can be removed, since we also have the 'alpha' method that does the same job.

121

This method can be removed as it does not make sense.

156–167

I am not so sure if many calls of the copy constructor are a problem. Can you provide me with a few examples of unnecessary/undesirable copying?

One more inline comment.

source/blender/freestyle/intern/blender_interface/BlenderFileLoader.cpp
585

It seems okay to me as well to use std::map. Just try to remove currentMindex and see if everything works.