[Bf-committers] Freestyle branch review

[Tamito KAJIYAMA]

Hello Brecht,

First of all, many thanks for the very careful review.  I am really grateful for the
high concentration of detailed analyses in many aspects of the Freestyle branch.

I understand that in most cases, the mentioned points are examples of many
similar elements that require fixes and improvements.  However, I think it is
appropriate to reply to all the discussed elements on a point-by-point basis,
so that questions and doubts are clarified and a solid basis for future plans is
settled.

> My impression from testing and reading code is that this feature is
> extremely flexible, and there's been some great renders, however the
> user interface and terminology are very technical and also
> inconsistent with other parts of Blender. If it would be a core
> feature, I think it should be designed from the point of view of
> adding a line drawing feature for the internal renderer, not as a way
> to get all freestyle settings available. Blender is not always
> consistent but for core features we should at least try to set a
> higher bar.

Thanks again for the comments and I fully agree with all the points.

> It could also become an external render engine, but it relies quite a
> lot on the internal renderer and integrates in some ways that are
> currently not possible though or python API. If the purpose of this is
> to make a straight integration of freestyle, then I think for a design
> point of view it would work best as an external engine, but from a
> technical point of view this seems challenging.

I share the same observations here.  In the beginning of the integration work,
the connection between Blender and Freestyle was rather loose.  Since then
the interaction between them has become tighter in order to permit Freestyle
to support advanced Blender functionalities such as full-sample antialiasing,
new data blocks for line styles, and extended mesh edge/face flagging.
Given the present level of tight coupling, transforming Freestyle into an
external rendering engine would be a challenge requiring major updates on
the Blender API side.

> UI NOTES:
> 
> * Many of the terms are very technical. For example the raycasting
> algorithm, from my understanding only two of these are recommed to
> use. So why not remove all algorithms but those two, and make it an
> on/off option to take out-of-view edges into account? This is just one
> example, there's a dozen such options which could get better names and
> descriptions. Some names might always remain a bit technical but at
> least the tooltip should give good clues then.

Concerning the raycasting algorithms, the recommended algorithms are
the Culled and Un-culled Cumulative Visibility Detection algorithms.
The other algorithms are kept for testing purposes (e.g., with regard to
visual compatibility and performance).  The two algorithms have been
well tested, and I thnk it is safe to remove the other algorithms.  Making
the two algorithms into a toggle button is absolutely a good idea.

Any further suggestions for improved naming and descriptions of technical
words are highly welcome.

> * Getting started could be easier, right now it's pretty hard to find.
> There's a Freestyle panel but you have to go into the post processing
> options to enable it (the Freestyle panel is not grayed out so it's
> not clear it is disabled). Why not make that a toggle button in the
> Freestyle panel header? Then you have to add a LineSet (could be done
> automatic on enable). Then the result renders shows lines but they're
> pretty strange on models that I've tested other than a cube. The way
> this should work is that you enable freestyle, and it gives you a
> reasonable result, from which you can then start tweaking.

The location of the Freestyle toggle button in the post processing options
has been a matter of discussion with a lot of criticism from branch users.
I also find it a bit bothersome that one has to go to the Post Processing
panel to enable Freestyle.  The present location of the toggle is however the
most logical one among those proposed so far.

The rationale of the present location of the toggle button is two-fold:
(1) it globally turns Freestyle on and off; and (2) other Freestyle-related UI
panels are associated with a specific render layer.

Individual render layers can have a distinct set of Freestyle parameters.
The Freestyle panel in the Render options is intended to accommodate
these per-layer parameters.  The Freestyle toggle in the Include secion of
the Layers panel is used for enabling/disabling Freestyle on a per-layer basis.
When Freestyle is disabled by the Include: Freestyle toggle in a render layer,
the Freestyle panel does not show up.  For this reason, it seems out of place
to put the global toggle button in the Freestyle panel header.

One possibility here is to change the default settings as follows: enabling
Freestyle in the Post Processing panel, and disabling the Include: Freestyle
toggle in the Layers panel.

I agree that the default settings of Freestyle-related parameters can be better.
Having a pre-defined line set with simple black lines would be of great help
for new users.  The present default settings is admittedly not well maintained,
as Freestyle-specific settings in startup.blend tend to be overwritten and lost
by updates of that file in the trunk.  The merge of the branch into the trunk
will solve this issue.

> * Most of the freestyle panels probably should be moved outside of the
> Render properties, right now it's pretty crowded there. It could
> become it's own tab perhaps, visible when freestyle is enabled? Also
> it might be good to only show things like "mark freestyle edge" when
> freestyle is enable, to avoid UI clutter.

I agree with these changes.  Some branch users have also proposed a
separate tab for Freestyle options.

> * General button placement, labels, etc should be improved. The way
> they are aligned and sized things looks a bit messy and inconsistent
> with the style used in other parts of Blender. Some buttons are also
> organized in strange ways, for example, why isn't the line sets list
> in the line sets panel?

The general comment is appreciated, and I welcome any further comments
and suggestions in detail.  Concerning the line sets list, I agree to move it to
the Line Sets panel.

> DATA DESIGN:
> 
> * The data that it adds to the Blender core is basically a LineStyle
> datablock, various RenderData and RenderLayer setttings, and per
> edge/face marking flags. Besides that there are some python style
> scripts.

I confirm these observations.

> * The distinction between python scripting mode and parameter edit
> mode seems quite drastic, it seems you can only use one or the other?
> Once you do this you can only add python scripts and nearly all the
> other options no longer have any influence. Maybe this is difficult
> because of the freestyle design, but it's a bit strange that you can
> no longer assign a style to a particular group of objects from the UI
> then.

The initial integration plan included only a very simple UI for specifying
Python style modules (referred to as the Python Scripting mode now).

Just as a side note, a Python style module defines line styles by means
of five basic operations: feature edge selection, joining (concatenation),
splitting, sorting and stroke creation.  The first four operations can be
carried out in any order, possibly many times.  The stroke creation must
be the last operation in the style module.

Python style modules are monolithic in the sense that everything is
done in one shot.  Style modules have no functionality to build UI
controls and allow users to interactively manipulate style parameters.
Instead, parameter values must be hard-coded in the Python scripts
(i.e., changing parameter values requires manual script editing).

There was an attempt to embed some UI commands in style modules
to construct UI controls, but this direction was abandoned because of
limited user interaction capabilities.

Then, following an increasing amount of requests for an artist-friendly
built-in GUI, the Parameter Editor mode was introduced.  Its goal is
two-fold: to provide artists with a set of well-defined interactive stylization
components, as well as to retain Freestyle's high programmability.

To this end, I have two stages of development in my mind.  In the first
stage, the Parameter Editor mode will be equipped with pre-defined static
UI controls such as buttons and sliders.  In the second stage, a fine-grained
scripting functionality is introduced.  The idea is split a monolithic Python
style module into small pieces and combine them with pre-defined UI
controls implemented in the first stage.  This will permit users to write one or
more small Python code snippets that carry out some stylization operations,
and use them instead of the pre-defined UI controls.

The development of the Parameter Editor mode is in the first stage at the
moment.  For now, the high programmability of Freestyle is maintained by
just keeping the old way for using Python style modules.  For this reason,
you may find it strange to have these two completely independent manners
of line stylization.  I expect this situation can be improved in the future.

The Python Scripting mode and the Parameter Editor mode can be mixed.
Individual render layers can be set to one of the two modes.

> * The Line Style data block feels a bit out of place between the other
> datablocks, maybe it should have a better name but I don't have an
> immediate suggestion. Mostly it is strange that this is separate from
> the python scripting mode and only works with parameter edit mode. If
> I were working on a project and wanted some datablock to reuse line
> drawing styles, I wouldn't care if the line style was written in
> python or not, both types should be contained in this datablock.

I fully agree that Python scripts can be part of Line Style datablocks.
In the second stage of the Parameter Editor mode development, Python
code snippets could also be part of the Line Style datablocks.

I am open to suggestions of better naming for the Line Style datablocks.

> * Python style module scripts are specified with their full paths, to
> the scripts folder that comes with the blender executable. This will
> not work when trying to use the .blend file on another computer with
> Blender installed in a different location. User written scripts should
> get relative paths, the built in ones I'm not sure about. Maybe they
> should be referred to only by their name, or work more as presets for
> users to write their own and place them next to the .blend file or in
> a text datablock.

These points are absolutely reasonable.  In fact, using text blocks instead
of external Python script files was a to-do item, but then the development
focus was shifted to the Parameter Editor mode.  I agree that text blocks
should be used to store Python style modules in .blend files.

> CODE:
> 
> * Overall the integration code looks well implemented. The way
> datablocks, edge/face marking, operators, properties etc were added
> seems ok.

I acknowledge these comments.

> * I did notice some code in blender modules not following style and
> naming conventions, but this is not so hard to fix.

I agree.

> * The blender_integration code in the freestyle module I didn't
> analyze completely yet, though it looks like it could use some
> refactoring to fit better. It's got some strange indentation, missing
> GPL license headers, etc too.

Coding style in the Freestyle code base is incosistent in many places, and
some style cleaning effort is necessary before the merge.

I also agree that the blender_integration code deserves some refactoring,
especially with regard to RNA-related functions.

> * Python script line style follow the freestyle API, which in terms of
> code style this is very different from the other API's in Blender. I'm
> not sure how strict we want to be here...

The Freestyle Python API has a peculiar naming convention, partly originating
from the old API implementation based on SWIG (plus the underlying C++
code base).  Improvements in this regard were also suggested by Campbell
in his code review results.  I agree that some efforts on style consistency with
the rest of the Blender Python API would be needed.

> So anyway, this is my personal impression, and it looks to me like
> this would not be ready for the 2.65 release. I'm not sure what the
> best way forward is, if the freestyle developers have time to address
> these issues or even agree with them, or if it's ok to keep it a
> branch, or if people think it should go in anyway without bigger
> changes. Besides the bigger design decisions I think there's a few
> pretty clear things that should be fixed for an official release
> anyway.

I share the same thoughs with you.  In my humble opinion, all the fixes and
improvements discussed above are worth being made before the merge.

For what concerns the time when the merge happens, I am completely open.
The best way to go is a matter of discussion, and I am more than willing to
hear opinions.

I am not so optimisitic about the amount of time required for the implementation
of the discussed elements, since I am the only active branch developer and I tend
to be time-constrained.  Hence I tend to avoid providing estimated arrival time.

That said, I have been able to keep working on the Freestyle branch, and I think
I am going to continue the integration work no matter what the decision is.

That's it for now.  Any further comments are greatly appreciated.

With best regards,

-- 
KAJIYAMA, Tamito <rd6t-kjym at asahi-net.or.jp>


-----Original Message----- 
From: Brecht Van Lommel 
Sent: Wednesday, October 24, 2012 5:20 PM 
To: bf-blender developers 
Subject: [Bf-committers] Freestyle branch review 

Hi all,

I was asked to look into the freestyle branch, to see what needs to be
done to get it included in an official release. For those not familiar
with how it works, there is some documentation here:
http://wiki.blender.org/index.php/User:Flokkievids/Freestyle

My impression from testing and reading code is that this feature is
extremely flexible, and there's been some great renders, however the
user interface and terminology are very technical and also
inconsistent with other parts of Blender. If it would be a core
feature, I think it should be designed from the point of view of
adding a line drawing feature for the internal renderer, not as a way
to get all freestyle settings available. Blender is not always
consistent but for core features we should at least try to set a
higher bar.

It could also become an external render engine, but it relies quite a
lot on the internal renderer and integrates in some ways that are
currently not possible though or python API. If the purpose of this is
to make a straight integration of freestyle, then I think for a design
point of view it would work best as an external engine, but from a
technical point of view this seems challenging.


UI NOTES:

* Many of the terms are very technical. For example the raycasting
algorithm, from my understanding only two of these are recommed to
use. So why not remove all algorithms but those two, and make it an
on/off option to take out-of-view edges into account? This is just one
example, there's a dozen such options which could get better names and
descriptions. Some names might always remain a bit technical but at
least the tooltip should give good clues then.

* Getting started could be easier, right now it's pretty hard to find.
There's a Freestyle panel but you have to go into the post processing
options to enable it (the Freestyle panel is not grayed out so it's
not clear it is disabled). Why not make that a toggle button in the
Freestyle panel header? Then you have to add a LineSet (could be done
automatic on enable). Then the result renders shows lines but they're
pretty strange on models that I've tested other than a cube. The way
this should work is that you enable freestyle, and it gives you a
reasonable result, from which you can then start tweaking.

* Most of the freestyle panels probably should be moved outside of the
Render properties, right now it's pretty crowded there. It could
become it's own tab perhaps, visible when freestyle is enabled? Also
it might be good to only show things like "mark freestyle edge" when
freestyle is enable, to avoid UI clutter.

* General button placement, labels, etc should be improved. The way
they are aligned and sized things looks a bit messy and inconsistent
with the style used in other parts of Blender. Some buttons are also
organized in strange ways, for example, why isn't the line sets list
in the line sets panel?


DATA DESIGN:

* The data that it adds to the Blender core is basically a LineStyle
datablock, various RenderData and RenderLayer setttings, and per
edge/face marking flags. Besides that there are some python style
scripts.

* The distinction between python scripting mode and parameter edit
mode seems quite drastic, it seems you can only use one or the other?
Once you do this you can only add python scripts and nearly all the
other options no longer have any influence. Maybe this is difficult
because of the freestyle design, but it's a bit strange that you can
no longer assign a style to a particular group of objects from the UI
then.

* The Line Style data block feels a bit out of place between the other
datablocks, maybe it should have a better name but I don't have an
immediate suggestion. Mostly it is strange that this is separate from
the python scripting mode and only works with parameter edit mode. If
I were working on a project and wanted some datablock to reuse line
drawing styles, I wouldn't care if the line style was written in
python or not, both types should be contained in this datablock.

* Python style module scripts are specified with their full paths, to
the scripts folder that comes with the blender executable. This will
not work when trying to use the .blend file on another computer with
Blender installed in a different location. User written scripts should
get relative paths, the built in ones I'm not sure about. Maybe they
should be referred to only by their name, or work more as presets for
users to write their own and place them next to the .blend file or in
a text datablock.


CODE:

* Overall the integration code looks well implemented. The way
datablocks, edge/face marking, operators, properties etc were added
seems ok.

* I did notice some code in blender modules not following style and
naming conventions, but this is not so hard to fix.

* The blender_integration code in the freestyle module I didn't
analyze completely yet, though it looks like it could use some
refactoring to fit better. It's got some strange indentation, missing
GPL license headers, etc too.

* Python script line style follow the freestyle API, which in terms of
code style this is very different from the other API's in Blender. I'm
not sure how strict we want to be here...


So anyway, this is my personal impression, and it looks to me like
this would not be ready for the 2.65 release. I'm not sure what the
best way forward is, if the freestyle developers have time to address
these issues or even agree with them, or if it's ok to keep it a
branch, or if people think it should go in anyway without bigger
changes. Besides the bigger design decisions I think there's a few
pretty clear things that should be fixed for an official release
anyway.

Brecht.