Uv tools

Uv tools created during GSOC 2011.

To see functionality included see:

http://wiki.blender.org/index.php/User:Psy-Fi

Stitch documentation may need some updating, but mostly correct for now check image/uv area header for instructions.

[psy-fi, 2011-10-31 13:54:10]

some corrections for load functions left over from onion

[psy-fi, 2011-11-01 15:14:17]

Avoid hang in static island resolving when redoing operator stitch

[dingto, 2011-11-02 15:09:28]

Some quick going over code style in rna/dna and UI scripts. 
Overall, looks good to me.

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/space_view3d_toolbar.py (right):

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_view3d_toolbar.py:1046: #lazy stuff, but
maybe not so, export size should be unified. Maybe add better description to
rna?
Maybe fix that still and remove the comment in the layout code?

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesdna/DNA_s...
File source/blender/makesdna/DNA_scene_types.h (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_scene_types.h:538: #define UV_SCULPT_TOOL_GRAB					3
Indentation

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_scene_types.h:541: #define
UV_SCULPT_TOOL_RELAX_HC			2
Indentation

[brechtvl, 2011-12-16 17:24:07]

Some initial comments from quick read over the code.

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/space_image.py (right):

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_image.py:56: layout.prop(uv,
"show_other_objects")
What is the reason for this change?

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/space_userpref.py (right):

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_userpref.py:426: col.prop(system,
"use_high_bit_depth_tex")
I think we should call this use_float_textures, seems more consistent with other
options in blender.

http://codereview.appspot.com/5306077/diff/7005/source/blender/blenlib/intern...
File source/blender/blenlib/intern/math_vector_inline.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/blenlib/intern...
source/blender/blenlib/intern/math_vector_inline.c:388: MINLINE float
len_squared_v2(const float v[2])
Declaration for this is not in BLI_math_vector.h.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_ops.c:1249: /* ********************** smart
stitch operator *********************** */
Could this be moved into a separate file uvedit_stitch.c?

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_parametrizer.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_parametrizer.c:218: DerivedMesh
*derivedMesh;
Rather than keeping the derivedmesh around, I'd rather see PVert.co store the
coordinate itself as float co[3]; instead of float *co;.  That should avoid the
need to do this.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_unwrap_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_unwrap_ops.c:1208: 
I don't think we should have this warning? If we do, it's not the size/dsize
that should be checked, but the matrix obmat[4][4] which contains all
transformations. It's also currently always giving warnings due to dsize being 1
now.

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
File source/blender/gpu/intern/gpu_draw.c (left):

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
source/blender/gpu/intern/gpu_draw.c:569: glTexImage2D(GL_TEXTURE_2D, 0, 
GL_RGBA,  rectw, recth, 0, GL_RGBA, GL_UNSIGNED_BYTE, rect);
Missing indent here and a few lines further.

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
File source/blender/gpu/intern/gpu_draw.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
source/blender/gpu/intern/gpu_draw.c:810: printf("papakia");
Remove debug print.

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesdna/DNA_s...
File source/blender/makesdna/DNA_scene_types.h (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_scene_types.h:802: typedef struct UVIslandComponent
{
This should not be in DNA, or is stored to file somewhere?

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_sculpt_paint.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_sculpt_paint.c:30: #include <GL/glew.h>
Is this needed?

[sergey.vfx, 2011-12-16 17:32:38]

Hi,

Made first itteration of review.

I don't think check of malloc() result is indeed helps, it's still not reliable
and makes code a bit difficult to read.

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/space_image.py (right):

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_image.py:294: layout.separator()
Picky: don;t think last separator is indeed needed.

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/space_view3d_toolbar.py (right):

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_view3d_toolbar.py:1048: row.prop(ipaint,
"screen_grab_size", text="")
Not sue why row is needed for just one property

http://codereview.appspot.com/5306077/diff/7005/source/blender/blenlib/intern...
File source/blender/blenlib/intern/math_vector_inline.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/blenlib/intern...
source/blender/blenlib/intern/math_vector_inline.c:388: MINLINE float
len_squared_v2(const float v[2])
Can't see this function in header file.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/mesh/e...
File source/blender/editors/mesh/editmesh_lib.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_lib.c:2335: uv = tf->uv[v->tfindex];
Picky thing: personally, i'm trying to follow existing code style in file. Here
it's a bit tricky because can see several styles used in this file. But maybe in
the new code it's possible to stick to one style? :)

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_lib.c:2482: if(!map || !stack ||
!islandbuf){
Don't actually think this checks are needed. There are too many laces where
blender will crash due to memory allocation issues. Also, this check means
almost nothing in linux -- memory manager is optimistic there and might allocate
less memory than it was requested hoping that it wouldn't be used fully (in case
there's no enough free memory on the moment of allocation).

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/object...
File source/blender/editors/object/object_edit.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/object...
source/blender/editors/object/object_edit.c:91: #include "ED_image.h"
Not sure why this is needed.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/screen...
File source/blender/editors/screen/screen_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/screen...
source/blender/editors/screen/screen_ops.c:366:
int	ED_operator_uvedit_no_uv_sculpt(struct bContext *C)
Picky: not really great name, imo. What does it mean "no"? Is it action of
check? Maybe it might be changed to ED_operator_uvedit_can_uv_sculpt?

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/screen...
source/blender/editors/screen/screen_ops.c:407:
int	ED_operator_uv_seam_tag_active(struct bContext *C)
Also picky: "tag" usually means action like "tagging to perform some action
later". This is more like check, not tag.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/sculpt...
File source/blender/editors/sculpt_paint/paint_image.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/sculpt...
source/blender/editors/sculpt_paint/paint_image.c:53: #include "BLI_editVert.h"
Why this is needed?

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/sculpt...
File source/blender/editors/sculpt_paint/paint_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/sculpt...
source/blender/editors/sculpt_paint/paint_ops.c:350:
WM_main_add_notifier(NC_BRUSH|NA_EDITED, brush);
Better to use WM_event_add_notifier() here

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/sculpt...
source/blender/editors/sculpt_paint/paint_ops.c:365: ot->exec =
brush_uv_sculpt_tool_set_exec;
Picky: "a= b" vs. "a = b" in the same new block of code

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/space_...
File source/blender/editors/space_view3d/view3d_draw.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/space_...
source/blender/editors/space_view3d/view3d_draw.c:2778: ar->regiondata;	char
err_out[256] = "unknown";
Not sure if it's failure of text formation in codereview, is this line indeed
formatted in such way?

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/space_...
source/blender/editors/space_view3d/view3d_draw.c:2826: /* set temporary new
size */
Looks a bit tricky =\

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_draw.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_draw.c:899: glPointSize(5.0);
We usually trying to restore changes made to ogl context. Can't see ifpoint size
is resetting to 1.0 in this function.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_intern.h (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_intern.h:75: #define UNWRAP_SCALE_EPSILON
0.00001
Maybe FLT_EPS might be used?

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_ops.c:693: UvElement
*get_uv_element(UvElementMap *map, EditFace *efa, int index){
For public functions we're trying to keep more verbose names. In this case it
might worth of addin ED_ prefix because this funciton is from editors module.
Same for some other functions from thins file.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_ops.c:1335: static StitchPreviewer
*_stitch_preview;
Uh, static global variable..

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_ops.c:1664: if(efa->tmp.l ==
STITCH_NO_PREVIEW)
Usually tmp isn't used to save any state and used as temporary storage for
single operator exec only. Not sure about better ways here tho..

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_unwrap_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_unwrap_ops.c:43: #include
"DNA_modifier_types.h"
Doesn't seems to be needed.

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
File source/blender/gpu/intern/gpu_draw.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
source/blender/gpu/intern/gpu_draw.c:602: GPUs (OpenGL version >= 2.0)since they
support non-power-of-two-textures */
Can remember recent fixes for such textures from Brecht. Did you check this with
him?

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_sculpt_paint.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_sculpt_paint.c:68: #include "ED_sculpt.h"
Maybe it's failure of codereview, but can't see usages of functions from this
modules.

[brechtvl, 2011-12-17 13:59:57]

Reviewed high bit depth texture part.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_draw.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_draw.c:931: ToolSettings *toolsettings =
CTX_data_scene(C)->toolsettings;
No need to get scene from context here, have scene pointer.

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
File source/blender/gpu/intern/gpu_draw.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
source/blender/gpu/intern/gpu_draw.c:433: float *color_corrected_frect = NULL;
Better call this srgb_frect instead of color_corrected_frect for clarity.

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
source/blender/gpu/intern/gpu_draw.c:488: {
Sorry for being picky, but the bracket code style is not consistent with the
file and itself. E.g. ){ => ) {, opening { on same line, } always on own line,
don't use {} for single statement, .. .

Also comments use lowercase and space between /* */, no camel case for
variables.

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/IMB_imbuf.h
File source/blender/imbuf/IMB_imbuf.h (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/IMB_imbu...
source/blender/imbuf/IMB_imbuf.h:374: void
IMB_float_color_corrected_from_float_rect(struct ImBuf *ibuf, float *buffer);
Better call this function something like IMB_float_to_srgb_float_rect or
something like that, color corrected isn't really the right term.

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
File source/blender/imbuf/intern/divers.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
source/blender/imbuf/intern/divers.c:336: float dither= ibuf->dither / 255.0f;
Don't need to dither in float => float conversion, that's for float => byte
conversion only.

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
source/blender/imbuf/intern/divers.c:378: }else if(ELEM(profile,
IB_PROFILE_NONE, IB_PROFILE_SRGB)) {
This doesn't make sense to me, the float buffer should be always be linear?
ibuf->profile can't apply to both the byte and float buffer?

At least in the render engine, the convention is that float = always linear, so
OpenGL is going to be consistent with that, ibuf->profile should not be checked.

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_userdef.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_userdef.c:2706: 
These are call half float textures elsewhere in the UI for EXR, could use that
term here too in description.

[psy-fi, 2011-12-21 17:55:49]

A few remarks taken care of

[psy-fi, 2011-12-21 17:56:20]

A few comments for the remarks

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/space_image.py (right):

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_image.py:56: layout.prop(uv,
"show_other_objects")
because in the branch I have added code to show uvs when in texture paint mode
too and it's nice to be able to toggle other objects when not editing uvs

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/space_userpref.py (right):

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_userpref.py:426: col.prop(system,
"use_high_bit_depth_tex")
Hmm...not sure about that. "Float" is misleading since internally I use 16 bits
integer format. I could change this to use half float but it's an extra
extension to check for, while 16 bit integer images are (theoretically)
supported since early OpenGL. Pros are that checking for the extension could
give visual feedback to user while current method simply fails silently for
systems that don't support 16 bit integer format. I don't really mind though,
your call.

http://codereview.appspot.com/5306077/diff/7005/source/blender/blenlib/intern...
File source/blender/blenlib/intern/math_vector_inline.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/blenlib/intern...
source/blender/blenlib/intern/math_vector_inline.c:388: MINLINE float
len_squared_v2(const float v[2])
I was going to use this for uv sculpting but looks like I ended up using the
normal length after all. Anyway, leaving this here for anyone interested and
adding to header.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_draw.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_draw.c:931: ToolSettings *toolsettings =
CTX_data_scene(C)->toolsettings;
Done

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_ops.c:1249: /* ********************** smart
stitch operator *********************** */

Done

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_parametrizer.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_parametrizer.c:218: DerivedMesh
*derivedMesh;
This seems a bit memory hungry, no? Is there any reason for not keeping the
derivedmesh (other than maybe some ugliness in the code itself?). I was thinking
that using derivedmesh for unwrap could be generalized in the future for other
modifiers as well.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_unwrap_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_unwrap_ops.c:43: #include
"DNA_modifier_types.h"
	SubsurfModifierData smd = {{0}}; and
	
	DerivedMesh *derivedMesh, *initialDerived;

down below use definitions from this file

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
File source/blender/gpu/intern/gpu_draw.c (left):

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
source/blender/gpu/intern/gpu_draw.c:569: glTexImage2D(GL_TEXTURE_2D, 0, 
GL_RGBA,  rectw, recth, 0, GL_RGBA, GL_UNSIGNED_BYTE, rect);
On 2011/12/16 17:24:07, brechtvl wrote:
> Missing indent here and a few lines further.

Done.

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
File source/blender/gpu/intern/gpu_draw.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
source/blender/gpu/intern/gpu_draw.c:433: float *color_corrected_frect = NULL;
On 2011/12/17 13:59:57, brechtvl wrote:
> Better call this srgb_frect instead of color_corrected_frect for clarity.

Done.

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
source/blender/gpu/intern/gpu_draw.c:488: {
No problem with being picky. Though I must say that single statement brackets is
an old habit. My java professor used to say "better have these from the start
than forget to add later". And I think he's right. Maybe I should contribute to
the ongoing style discussion about this but I will change to accepted style for
now.

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
source/blender/gpu/intern/gpu_draw.c:602: GPUs (OpenGL version >= 2.0)since they
support non-power-of-two-textures */
I didn't see any functionality like that when I looked into it, but I'll ask
him.

http://codereview.appspot.com/5306077/diff/7005/source/blender/gpu/intern/gpu...
source/blender/gpu/intern/gpu_draw.c:810: printf("papakia");
On 2011/12/16 17:24:07, brechtvl wrote:
> Remove debug print.

Done.

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
File source/blender/imbuf/intern/divers.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
source/blender/imbuf/intern/divers.c:336: float dither= ibuf->dither / 255.0f;
I wasn't certain how to proceed here since this was supposed to replicate the
result seen already, but I agree, so I will delete the dither from the files
here

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
source/blender/imbuf/intern/divers.c:378: }else if(ELEM(profile,
IB_PROFILE_NONE, IB_PROFILE_SRGB)) {
Well, don't look at me, I just copied the code from the above functions ;).
Apart from that, it's not always 100% certain that we get a linear buffer. For
example when creating a new float image from within blender, its
IMB_PROFILE_LINEAR flag isn't set. This leads to bug
http://projects.blender.org/tracker/index.php?func=detail&aid=28193&group_id=...
for instance. I agree that this should be enforced, but I don't know how to
better do this.

[brechtvl, 2011-12-22 14:48:19]

On 2011/12/21 17:56:20, psy-fi wrote:
>
http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
> File release/scripts/startup/bl_ui/space_userpref.py (right):
> 
>
http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
> release/scripts/startup/bl_ui/space_userpref.py:426: col.prop(system,
> "use_high_bit_depth_tex")
> Hmm...not sure about that. "Float" is misleading since internally I use 16
bits
> integer format. I could change this to use half float but it's an extra
> extension to check for, while 16 bit integer images are (theoretically)
> supported since early OpenGL. Pros are that checking for the extension could
> give visual feedback to user while current method simply fails silently for
> systems that don't support 16 bit integer format. I don't really mind though,
> your call.

Ah, the way I see this is that "float" is not about the OpenGL texture data
format, but about the fact that you use float information from the image to
create the texture. Not sure what to call the option to make this distinction
clear, but it can be explained like this in the tooltip.

>
http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
> File source/blender/editors/uvedit/uvedit_parametrizer.c (right):
> 
>
http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
> source/blender/editors/uvedit/uvedit_parametrizer.c:218: DerivedMesh
> *derivedMesh;
> This seems a bit memory hungry, no? Is there any reason for not keeping the
> derivedmesh (other than maybe some ugliness in the code itself?). I was
thinking
> that using derivedmesh for unwrap could be generalized in the future for other
> modifiers as well.

Storing 3 floats instead of a 64 bit pointers only goes from 8 to 12 bytes.
Probably keeping around the derivedmesh gives a higher memory peak together with
unwrap solver that is used later. If there's a reason to keep it around, ok, but
it's not clear to me why you'd want to keep it around except for this co
pointer.

>
http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
> File source/blender/imbuf/intern/divers.c (right):
> 
>
http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
> source/blender/imbuf/intern/divers.c:378: }else if(ELEM(profile,
> IB_PROFILE_NONE, IB_PROFILE_SRGB)) {
> Well, don't look at me, I just copied the code from the above functions ;).
> Apart from that, it's not always 100% certain that we get a linear buffer. For
> example when creating a new float image from within blender, its
> IMB_PROFILE_LINEAR flag isn't set. This leads to bug
>
http://projects.blender.org/tracker/index.php?func=detail&aid=28193&group_id=...
> for instance. I agree that this should be enforced, but I don't know how to
> better do this.

That needs to be fixed indeed, but it's a separate issue, just leave out the
checks for profile so that we're compatible with render, we can fix other
functions later.

[brechtvl, 2011-12-22 14:58:07]

Testing the UV unwrap subsurf function, I can't really see enabling subsurf
giving better results than leaving it disabled. It's always a bit different but
not clearly lower distortion? Do you have examples where we can see the
improvement?

[psy-fi, 2011-12-22 15:38:58]

Hi brecht, it helps on subdivided meshes on areas with high curvature. I have
attached a sample blend file here 

http://www.pasteall.org/blend/10449

which is a standard cube, unwrapped but compressed so that curvature becomes
more of a issue. If you tick subsurf you'll see that the the texture space gets
better distributed in these areas when you tick subsurf unwrap while with it you
get stretching.

[psy-fi, 2011-12-22 15:52:35]

Gaah! I meant -without- you get stretching. I know it's not a big effect. And
you also get some rotation usually which I didn't like a lot. I wouldn't use it
often myself either though a couple of people have mentioned that it makes
better unwraps. I really don't have something better to show for it though. It
was on the todo list so I added it. The idea was to get the unwrap you'd get if
you had subsurfed the mesh and it does that indeed (if you subsurf, apply then
unwrap you get the same result)

[psy-fi, 2011-12-22 15:57:34]

Hi brecht, another thing, just remembered that I need to actually keep the
derivedMesh because when flushing the solver I needed a place to write the uvs
to. Haven't checked with a dummy float [2] array though, this could work too.

[brechtvl, 2011-12-22 16:39:36]

Tested the UV tools.

I'm not convinced UV sculpt is a feature we should have in trunk, it seems
overkill.. Grab is like proportional editing, and instead of Pinch/Relax
minimize stretch is more useful. The sculpt falloff curve is also too much, UV
editing isn't really an artistic thing, it's a matter of getting the textures
mapped with minimal stretching. This seems a step too far to me.

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/scree...
File source/blender/editors/screen/screen_ops.c (right):

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/scree...
source/blender/editors/screen/screen_ops.c:366:
int	ED_operator_uvedit_no_uv_sculpt(struct bContext *C)
I'd just move this poll and the two others here to the files where they are
used., they are quite specialized.

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/uvedi...
File source/blender/editors/uvedit/uvedit_ops.c (right):

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/uvedi...
source/blender/editors/uvedit/uvedit_ops.c:3148: /* flags to determine if we uv
is separated from first editface match */
Indentation and space between ){

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/uvedi...
source/blender/editors/uvedit/uvedit_ops.c:3310: static void
UV_OT_mark_seam_uv(wmOperatorType *ot)
This operator was very confusing to me, it took me 10 minutes to figure out how
to use it. Why not just make a function to mark selected edges as seams, that
works regardless of sync selection? Making this depend on the edge select mode
option in 3d view and mouse position doesn't seem right.

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/uvedi...
File source/blender/editors/uvedit/uvedit_smart_stitch.c (right):

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/uvedi...
source/blender/editors/uvedit/uvedit_smart_stitch.c:1844: void
UV_OT_stitch(wmOperatorType *ot)
I found the options presented quite confusing, what I would do is:

* Remove the preview toggle option, just always show it.
* Replace (ctrl+V) by shift+select mouse and make it toggle, same as outside of
this mode.
* Don't show limit adjust unless limit is enabled.
* Enable snap by default
* Format header text like:

(S)nap On, (M)idpoints Off, (L)imit 0.01 (Ctrl Wheel adjust), Switch (I)sland,
shift select vertices

The vertex/edge stitching I can't really understand on a user level, it's not
clear to me what it does exactly, not sure why yet.

[brechtvl, 2011-12-22 16:54:14]

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/inclu...
File source/blender/editors/include/ED_uvedit.h (right):

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/inclu...
source/blender/editors/include/ED_uvedit.h:90: /* uvedit_island_manager.c */
There is no uvedit_island_manager.c.

http://codereview.appspot.com/5306077/diff/34002/source/blender/makesdna/DNA_...
File source/blender/makesdna/DNA_scene_types.h (right):

http://codereview.appspot.com/5306077/diff/34002/source/blender/makesdna/DNA_...
source/blender/makesdna/DNA_scene_types.h:905: * UVIsland Component, stores
boundaries for uv island, possibly a list of faces too, though this would be
This doesn't seem to be in use anymore?

[brechtvl, 2011-12-22 17:02:56]

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/sculp...
File source/blender/editors/sculpt_paint/paint_image.c (right):

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/sculp...
source/blender/editors/sculpt_paint/paint_image.c:5820: 
What is the use case for these tools, it seems like the kind of thing that would
be an addon, not a core Blender feature?

Baking vertex group weights to an image seems like the kind of thing you'd do
with the render engine. There can be of course someone who can use this, but to
me it just doesn't fit overall this way, why have a tool to do specifically
this?

[psy-fi, 2012-01-04 18:22:34]

Most remarks taken care of, yet another patch coming soon.

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/space_image.py (right):

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_image.py:294: layout.separator()
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Picky: don;t think last separator is indeed needed.

Indeed it's not :). Done.

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/space_view3d_toolbar.py (right):

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_view3d_toolbar.py:1046: #lazy stuff, but
maybe not so, export size should be unified. Maybe add better description to
rna?
On 2011/11/02 15:09:28, dingto wrote:
> Maybe fix that still and remove the comment in the layout code?

Tool didn't pass review from brecht so removing

http://codereview.appspot.com/5306077/diff/7005/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/space_view3d_toolbar.py:1048: row.prop(ipaint,
"screen_grab_size", text="")
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Not sue why row is needed for just one property

removed

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/mesh/e...
File source/blender/editors/mesh/editmesh_lib.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_lib.c:2335: uv = tf->uv[v->tfindex];
did some changes for that.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/mesh/e...
source/blender/editors/mesh/editmesh_lib.c:2482: if(!map || !stack ||
!islandbuf){
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Don't actually think this checks are needed. There are too many laces where
> blender will crash due to memory allocation issues. Also, this check means
> almost nothing in linux -- memory manager is optimistic there and might
allocate
> less memory than it was requested hoping that it wouldn't be used fully (in
case
> there's no enough free memory on the moment of allocation).

Just copying the paradigm set by the above function. Removed.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/screen...
File source/blender/editors/screen/screen_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/screen...
source/blender/editors/screen/screen_ops.c:366:
int	ED_operator_uvedit_no_uv_sculpt(struct bContext *C)
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Picky: not really great name, imo. What does it mean "no"? Is it action of
> check? Maybe it might be changed to ED_operator_uvedit_can_uv_sculpt?

Done.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/screen...
source/blender/editors/screen/screen_ops.c:407:
int	ED_operator_uv_seam_tag_active(struct bContext *C)
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Also picky: "tag" usually means action like "tagging to perform some action
> later". This is more like check, not tag.

Tag refers to "uv seam tag". Changed to ED_operator_uv_seam_tag_check_active,
though this function will most probably go away later.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/sculpt...
File source/blender/editors/sculpt_paint/paint_image.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/sculpt...
source/blender/editors/sculpt_paint/paint_image.c:53: #include "BLI_editVert.h"
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Why this is needed?

uv_sculpt_brush_poll in the same file uses EditMesh. I can't seem to be able to
look at it in code review though :/

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/sculpt...
File source/blender/editors/sculpt_paint/paint_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/sculpt...
source/blender/editors/sculpt_paint/paint_ops.c:350:
WM_main_add_notifier(NC_BRUSH|NA_EDITED, brush);
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Better to use WM_event_add_notifier() here

Done.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/sculpt...
source/blender/editors/sculpt_paint/paint_ops.c:365: ot->exec =
brush_uv_sculpt_tool_set_exec;
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Picky: "a= b" vs. "a = b" in the same new block of code

Done.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/space_...
File source/blender/editors/space_view3d/view3d_draw.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/space_...
source/blender/editors/space_view3d/view3d_draw.c:2778: ar->regiondata;	char
err_out[256] = "unknown";
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Not sure if it's failure of text formation in codereview, is this line indeed
> formatted in such way?

Weight paint tools got rejected by brecht, not relevant anymore.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/space_...
source/blender/editors/space_view3d/view3d_draw.c:2826: /* set temporary new
size */
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Looks a bit tricky =\

Same as above.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_draw.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_draw.c:899: glPointSize(5.0);
On 2011/12/16 17:32:38, sergey.vfx wrote:
> We usually trying to restore changes made to ogl context. Can't see ifpoint
size
> is resetting to 1.0 in this function.

It's done at the end of the function. Changed this to use the pointsize
variable.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_intern.h (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_intern.h:75: #define UNWRAP_SCALE_EPSILON
0.00001
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Maybe FLT_EPS might be used?

Yes,used FLT_EPSILON from float.h.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
File source/blender/editors/uvedit/uvedit_ops.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_ops.c:693: UvElement
*get_uv_element(UvElementMap *map, EditFace *efa, int index){
On 2011/12/16 17:32:38, sergey.vfx wrote:
> For public functions we're trying to keep more verbose names. In this case it
> might worth of addin ED_ prefix because this funciton is from editors module.
> Same for some other functions from thins file.

Done.

http://codereview.appspot.com/5306077/diff/7005/source/blender/editors/uvedit...
source/blender/editors/uvedit/uvedit_ops.c:1335: static StitchPreviewer
*_stitch_preview;
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Uh, static global variable..

Yes, I kinda followed the sort of
module-private-variable-and-getter-type-function style here.

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/IMB_imbuf.h
File source/blender/imbuf/IMB_imbuf.h (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/IMB_imbu...
source/blender/imbuf/IMB_imbuf.h:374: void
IMB_float_color_corrected_from_float_rect(struct ImBuf *ibuf, float *buffer);
On 2011/12/17 13:59:57, brechtvl wrote:
> Better call this function something like IMB_float_to_srgb_float_rect or
> something like that, color corrected isn't really the right term.

reused IMB_buffer_float_from_float from your recent imbuf cleanup commit.

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
File source/blender/imbuf/intern/divers.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
source/blender/imbuf/intern/divers.c:336: float dither= ibuf->dither / 255.0f;
On 2011/12/21 17:56:20, psy-fi wrote:
> I wasn't certain how to proceed here since this was supposed to replicate the
> result seen already, but I agree, so I will delete the dither from the files
> here

This and the next comment no longer apply since I reuse
IMB_buffer_float_from_float

http://codereview.appspot.com/5306077/diff/7005/source/blender/imbuf/intern/d...
source/blender/imbuf/intern/divers.c:378: }else if(ELEM(profile,
IB_PROFILE_NONE, IB_PROFILE_SRGB)) {
No longer applies, see above

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesdna/DNA_s...
File source/blender/makesdna/DNA_scene_types.h (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_scene_types.h:538: #define UV_SCULPT_TOOL_GRAB					3
On 2011/11/02 15:09:28, dingto wrote:
> Indentation 

Done.

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_scene_types.h:541: #define
UV_SCULPT_TOOL_RELAX_HC			2
On 2011/11/02 15:09:28, dingto wrote:
> Indentation

Done.

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_scene_types.h:802: typedef struct UVIslandComponent
{
On 2011/12/16 17:24:07, brechtvl wrote:
> This should not be in DNA, or is stored to file somewhere?

Removed these. These are left over from an incomplete tool.

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_sculpt_paint.c (right):

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_sculpt_paint.c:30: #include <GL/glew.h>
On 2011/12/16 17:24:07, brechtvl wrote:
> Is this needed?
It's not. Done.

http://codereview.appspot.com/5306077/diff/7005/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_sculpt_paint.c:68: #include "ED_sculpt.h"
On 2011/12/16 17:32:38, sergey.vfx wrote:
> Maybe it's failure of codereview, but can't see usages of functions from this
> modules.

Removing those. I really should be wary of these includes from now on.

[psy-fi, 2012-01-05 23:55:27]

patch coming in a few minutes, stitch needs some more work to behave better on
strange setups but most stuff is ready. Brecht waiting comments on how to handle
uv brushes, check the emails I have sent for some considerations too. Still my
personal preference is a key to quickly get in and out of uv sculpt mode.

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/inclu...
File source/blender/editors/include/ED_uvedit.h (right):

http://codereview.appspot.com/5306077/diff/34002/source/blender/editors/inclu...
source/blender/editors/include/ED_uvedit.h:90: /* uvedit_island_manager.c */
On 2011/12/22 16:54:14, brechtvl wrote:
> There is no uvedit_island_manager.c.

Removed.

http://codereview.appspot.com/5306077/diff/34002/source/blender/makesdna/DNA_...
File source/blender/makesdna/DNA_scene_types.h (right):

http://codereview.appspot.com/5306077/diff/34002/source/blender/makesdna/DNA_...
source/blender/makesdna/DNA_scene_types.h:905: * UVIsland Component, stores
boundaries for uv island, possibly a list of faces too, though this would be
On 2011/12/22 16:54:14, brechtvl wrote:
> This doesn't seem to be in use anymore?

Removed. Left over from an unimplemented feature.

[psy-fi, 2012-01-06 00:04:50]

patch with changes so far.

[brechtvl, 2012-01-10 21:40:37]

Reviewed latest version of branch now, and committed some simple tweaks and
fixes, since the latest changes are not in codereview.

Latest version looks good, stitch tool is nicely improved. Still think UV Sculpt
should not be a mode but don't want to be too annoying about this... so I guess
it's acceptable. One comment about that is, what is Laplacian and HC relax/pinch
mode? That should get a better name, or at least get explained in the tooltip
what the difference is?

[psy-fi, 2012-01-10 22:07:06]

Hi Brecht, excellent, thanks a lot for the help! What about grab brushes for uv
sculpt? Also, I was thinking to also bring back the edge mode on stitching. The
rationale is that edges have a 1-1 correspondence between islands while uv
vertices can sometimes be selected across islands you may not need.

About HC/Laplacian, they produce a different result, though I can't seem to
think a proper name for them...Laplacian produces more uniform relaxation while
HC produces more concentrated and tense result at the edges of the island. If
only one is to be kept I would keep the Laplacian. I used both the algorithms
that Raul had made to see how they behaved. I don't think someone will have a
reason to use HC though.

I will look for any leftovers in the comments that need addressing(shouldn't be
a lot).