Page MenuHome

Texture Marks for freestyle strokes
ClosedPublic

Authored by Paolo Acampora (pkrime) on Jan 23 2014, 1:02 AM.

Diff Detail

Event Timeline

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).Jan 26 2014, 7:50 PM

this new patch supports:

  • library linking
  • copy/paste button
  • shader argument is a pyobject
  • coordinates spacing factor
  • one material per strokerep

I've been overzealous with the last point: creating a material for each strokerep is too much (suzanne only is rendered with about 40 identical materials!), it's not accettable. The current state with stroke mesh materials is provisional, I guess I'll have too fill a list of some sort and create new materials only when needed. There is time for optimization though.

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).Jan 28 2014, 3:31 PM

Because of some text undo the Stroke constructor was missing the init value of texStep, leading to divisions by zero and subsequent crashes when no textures are used. Now it's fixed, but we should actually move to a numeral system where division by zero is allowed and solve the issue for ever! :D

This version of the patch iterates through freestyle_bmain->mat to release materials (the former used mesh->mat). I was just thinking to some materials linked list to optimize the material generation, and now that I know there is one already I think I am halfway.

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).Jan 28 2014, 6:35 PM

fix "materials spam issue", now strokes with the same textures (or equally no-textures) use the same materials.

also UVs are computed only when needed, and C++ comments use proper format.

Now the patch is feature complete to me, last question to address is what kind of MTex are passable, right now BlenderTextureShader accepts LineStyleTextureSlot type only. We could add virtually any MTex without much efforts so it's only a matter of deciding what to accept.

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).Jan 28 2014, 9:22 PM

small update to keep up with recent api change

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).Feb 6 2014, 2:23 PM

fix for lame coding leading to crash on osx (and presumably windows)

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).Mar 4 2014, 6:03 PM

Loops some common operations inside the computeTexCoordWithTips, checks for valid range of vertex index to avoid crash when geometry modifiers alter the number of vertices, the tex spacing value is now from 0.1 to 100.0.

Tamito Kajiyama (kjym3) requested changes to this revision.Apr 10 2014, 5:48 AM

Hi Paolo,

Thanks a lot for the continued effort on the support for textured strokes in Freestyle!

I did the first round of code review. The implementation looks good in general, and I confirmed that the code works great.

My only concern at the moment is the place to put texture related options in the GUI. Now texture slot options are placed in the Properties window > Render Layers tab > Freestyle Line Style panel > Texture tab. I personally found this place intuitive. At the same time, this place is not so consistent with similar IDs having texture slots. Specifically, Material and World have texture slots, and these IDs have their own sub-tab in the Properties window > Texture tab. Line style texture slots are now shown in the other "other data textures" sub-tab, but it would make sense to introduce another (say, "Line Style") sub-tab and put the list of texture slots and their parameters there, in line with those in the Material and World sub-tabs. This seems more consistent with the existing texture-related Blender GUI components. I would like to get some other core Blender developers involved to review this part of the patch set.

For minor code revision requests, please take a look at inline comments for your evaluation.

release/scripts/freestyle/modules/parameter_editor.py
1348

The magic number 18 can be avoided by looping over items instead of item indices:

for slot in linestyle.texture_slots:
    if slot is not None:
        ...
source/blender/freestyle/intern/python/StrokeShader/BPy_BlenderTextureShader.h
23

You might want to add the \author tag here (and in the other added files) to attribute the work to you :)

source/blender/freestyle/intern/stroke/Stroke.h
50

This should be:

extern "C" {
#include "DNA_material_types.h"
}

The definition of MAX_MTEX could be after these lines.

541

Should be:

MTex *_mtex[MAX_MTEX];

This header file is not part of makedna and makesrna, so it is okay to use the symbol instead of the magic number.

757

The situation with no empty slot available is considered an error condition.
It would be helpful from artists' view point to detect such a situation and raise a run-time error.
So this method should return an error code (0 for success and -1 for failure, for example), and
BlenderTextureShader::shade() should check it.

source/blender/freestyle/intern/stroke/StrokeRep.h
40

These two lines should be replaced with the following:

extern "C" {
#include "DNA_material_types.h"
}
185

This line should be:

MTex *_mtex[MAX_MTEX];
Paolo Acampora (pkrime) updated this revision to Unknown Object (????).Apr 13 2014, 7:04 PM

First correction after code review, addressing magic numbers and declarations. No changes affecting usage, or UI yet.

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).Apr 19 2014, 2:51 PM

So silly of me, last diff I put on revision was a "reverse", wrong git command! o_O

This diff should comply with latest changes in trunk.

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).Apr 21 2014, 8:46 PM

Some UI tweaks by kjym3

Regarding the UI: my suggestion is to be consistent with other parts of Blender that use textures, and so do editing of the texture in the texture properties. This system could certainly be better, but I think it's better to be consistent than each module doing their own workaround for a deeper problem.

The big panel exposes only part of the texture properties anyway, so it doesn't seem to me such a big advantage to have that in the render layers tab when you need to switch to the texture tab for the rest anyway. A button to quickly jump from the render layer tab to the texture tab with the right texture selected also makes switching relatively fast I think. Further, with the texture properties in a separate tab there is that advantage that you can open a second properties editor for it, that's quite a common workflow for material editing I think.

The layout of the properties in the big panel could then perhaps also be made more consistent with the corresponding texture properties panels for materials etc, to make it easier to recognize what's what.

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).Apr 30 2014, 12:56 AM

Moved options to texture context, added a nodetree attribute to linestyles. Looks like the last one is needed for node textures, but I must admit I'm not sure.

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).May 1 2014, 12:09 AM

kjym3 fixed pinning and context resolution

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).May 1 2014, 1:28 AM

Coordinate parameters were missing in the UI

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).May 1 2014, 1:45 AM

Default value for texture spacing

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).May 1 2014, 4:00 PM

fix u error, use_texture flag, everything thanks to kjym3

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).May 1 2014, 11:37 PM

UI and underlying functions for node textures, by kjym3

I think the patch is ready for code review by other core Blender developers.

Brecht,

Could you please review the updated patch, this time from a general perspective?

The patch has been revised to address your comments on UI design. Now all per-texture Freestyle settings are present in the Texture properties context.

A few additions of relevance to the new round of code review include:

  • Pinning -- Line style ID data blocks now can be pinned while users move around different properties contexts. Since line styles do not have its own context right now, the active line style is pinned when pinning is done in the Render Layers context (previously the current scene was pinned instead). In other scene-related contexts (namely, in the Render and Scene context) pinning works on the current scene as previously. There is no pinnable ID data blocks in the Render Layers context except for line styles, so I believe this behavior change is acceptable.
  • Texture nodes -- Node-based textures can be used for textured strokes in Freestyle now. The addition seems straightforward, just following the existing texture workflow.

The code outside the freestyle module looks good to me, seems to be integrated nicely consistent with other data types.

source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
356–358

What's the purpose of this line? Should it be activated at some point in the future, maybe leave a comment if so?

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).May 2 2014, 7:28 PM

removing ifdef-ed lines

Hi brecht,
That was a forgotten ifdef, kjym3 revised the patch and made it better, he was kind enough to ifdef my code instead than deleting it, and I've been lazy enough to leave it there. Here we go with deletion, thank you!

Paolo Acampora (pkrime) updated this revision to Unknown Object (????).May 3 2014, 12:24 AM

Update to keep up with changes in trunk