Shader and interface to use blender textures on freestyle strokes
Details
Details
- Reviewers
Tamito Kajiyama (kjym3) Brecht Van Lommel (brecht) - Group Reviewers
Freestyle - Maniphest Tasks
- T38327: Texture Marks from blender textures
- Commits
- rBSb7f085d9c128: Patch D246: Texture Marks for freestyle strokes, written and contributed by…
rBACb7f085d9c128: Patch D246: Texture Marks for freestyle strokes, written and contributed by…
rBb7f085d9c128: Patch D246: Texture Marks for freestyle strokes, written and contributed by…
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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 | ||
| 22 | 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. | |
| 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]; | |
Comment Actions
First correction after code review, addressing magic numbers and declarations. No changes affecting usage, or UI yet.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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? | |
Comment Actions
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!