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 <[email protected]> -----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. _______________________________________________ Bf-committers mailing list [email protected] http://lists.blender.org/mailman/listinfo/bf-committers
