[Bf-committers] Unlimited clay patch review

[Sergey I. Sharybin]

  Hi, Blender Community!

I've reviewed unlimited clay patch yesterday. I haven't been able to 
apply patch/compile correctly, so i can't give feedback about how things 
are working, but i could give some feedback about patch itself.

- First of all it's created against a bit outdated version of trunk -- 
some files were moved, some functions renamed and so. It lead to plenty 
of rejected hunks in patchs.
- Patch contains plenty of non-functional changes: whitespace changes, 
somewhere "styling" changes, somewhere changed "logic" -- code in 
non-unlimitedclay related code was replaced with different code whick 
makes the same things (like normals in getEditMeshDerivedMesh). This 
makes patch much more difficult to be applied against newer trunk and 
readability of this patch also lower -- can't split what is actually 
changes and what's not.
- That including of "ED_mesh.h" in DerivedMesh.c and pbvh.c. It's not 
only ugly usage of absolute path. but it's also a breaking of 
archeticure -- blenkernel/ and blenlib/ shouldn't use editors/. I 
suppose editmesh is needed for easier changing mesh topology, but i also 
suppose it could be made without such hacks. For example add some 
utility functions to blenkernel/intern/mesh which would provide list of 
verts/edges/faces (similar lists as in editmesh). I think it's the most 
worth thing for which EditMesh is used atm.
- I also not sure why it's needed to move structures (like 
EditMeshDerivedMesh, StrokeCache, PBVH, PBVHNode and so) from .c-fiels 
into headers. This structures aren't supposed to be reused outside of 
their module and they should be a blackbox for other modules.
- Styling should be checked. I'd prefer to keep only one style inside 
one module. Check comments, brackets, spaces and so.
- Also it looks like there's some unused code adding by patch but which 
isn't used.
- I'm not fully like all that new fields inside EditVert. Maybe they 
could be moved inside tmp union or EditVert->data could be reused? Or as 
i mentioned, EditMesh could be replaced by something more light and 
common... Or maybe it'll be special structure for unlimited clay 
purposes and functions like {make,load}_clayMesh?
- I'm not sure why stuff like BLI_pbvh_iter_end should be changed?

That's all feedback i could give atm.

We discussed a bit ways to split out unneeded changes with Tom, but not 
sure that ways would be useful. I'd prefer if manual reading of patch 
would be done -- in this case all outdated stuff would be found.

I hope it'll help to make patch better and acceptable to be commited.

-- 
With best regards, Sergey I. Sharybin