Details

Reviewers

Campbell Barton (campbellbarton)
Liam Mitchell (CommanderCorianderSalamander)

Group Reviewers

Modeling

Maniphest Tasks

T37879: UV coordinates for Mesh primitives

Commits

rBS3c6709a63c26: Fix T37879: Default UV generation for mesh primitives.
rB3c6709a63c26: Fix T37879: Default UV generation for mesh primitives.

Summary

As per T37879, this patch adds default-generated UVs to mesh primitives (cone, cylinder, icosphere, uvsphere, cube, circle, grid) when they are added to the scene, since some of them can be pretty awkward to unwrap manually.

This is my first Blender patch so some feedback is welcome - most particularly:

Is this the right place for this stuff? I could find nowhere really better to put it but the functions I added feel kind of out of place where they are for some reason.
Is there an easier way to do it?
Should this be an option in the operator instead of a default? I can't think of any time when I'm adding a primitive that I wouldn't want UVs generated - even if I'm not going to use them I'll just unwrap it again myself, which is exactly what I would have done anyways - but it wouldn't be hard to implement that way I guess.

Diff Detail

Event Timeline

Reviewers are supposed to be project members/module owner. For now, I'm just like you, someone who'd like to help developping Blender.

Same here, I'm just a user. Not only that, I'm also not fit to review your code - although I'd love to test the implementation if you'd make a build!

michael knubben (michaelknubben) resigned from this revision.Apr 23 2014, 11:02 PM

I just tested your patch and it works as expected. The spherical ones could be better though.

As I already said, I'm not the best placed to be reviewer but I still put an inline comment.

source/blender/bmesh/operators/bmo_primitive.c
689	Code style: use 0.5f, not .5

Oops, I forgot to tell that the plane one is missing from your implementation, just add

ED_mesh_uv_texture_add(obedit->data, NULL, true)

to add_primitive_plane_exec in editmesh_add.c.

Grid and Plane are the same, except that grid is subdivided by default.

Updated sphere UVs a little bit - icospheres are a lot easier to work with now I think, and uvspheres pretty much the same - as well as making it work for planes too ^^

Can I even make a build, or do I need git access? And if I can, can anyone point me to something that tells me how? =)

This patch is a nice start, some notes:

When adding in editmode, its editing the UV's of all other existing geometry too.
Adding UV's should eventually be optional (though to get the patch tested and working its fine not to do this right away).
The code assumes tris in some places, suggest to add BLI_assert when assumptions are not met, to avoid failing silently.
Code style is not to Blender convention, (indentation, brace placement), see: http://wiki.blender.org/index.php/Dev:Doc/Code_Style

source/blender/bmesh/intern/bmesh_operators.h
141	picky in BMesh the term `create` is used for making new data. suggest `BM_mesh_calc_uv_sphere` `BM_mesh_calc_uv_cube` ... etc.
source/blender/bmesh/operators/bmo_primitive.c
294	Better make caller responsible for ensuring BMesh has UV's, This goes for all other similar functions too.
520	`alloca` wont be freed until the function exits. you have to be very careful calling alloca in a loop, and in this case it can easily run out of stack memory and crash. In this case you could move the body of the `for` loop into a static function.
898	`0.24f` could be made a `const float` and assigned a value. Same for other values re-used many times in this patch.

Campbell Barton (campbellbarton) requested changes to this revision.Apr 24 2014, 12:50 PM

Thanks @Campbell Barton (campbellbarton), updated the code with your suggested changes, as well as wildly simplifying cone and circle UV generation code (brought 80 lines or so down to about 10 =D)... making it work with lots of primitives on the same mesh was a little awkward at first but it should all be OK now, I think I figured it out. Take another look when you have time =)

Also haven't worked on adding the option to the operator panel yet - though I hope that shouldn't be too hard =)

Added option in the operator panel

Uploaded the wrong diff =(

Second round of review.

General note: Id like to avoid having the common case for adding primitives getting more complicated, If there are extra calculations to be made for UV's, they could check if (create_uvs) {... extra calculations ...}

source/blender/bmesh/operators/bmo_primitive.c
551	This is around `4440` degrees, This could be `2.1` since the value is in radians.
798	With all this effort to calculate the normals, probably better just to call. `BM_face_calc_normal`
896	could just pass this in.
939	can use the dot product here.
source/blender/editors/object/object_add.c
304	This could be optional, but instead, could probably add `ED_object_add_mesh_props` since this only applies to meshes.

Removed unnecessary code and cleaned up a little

One more bit of cleanup

Round three @Campbell Barton (campbellbarton), thanks for spending so much time reading my bad C ^.^

Assigning to Mont29, this is fairly significant change still - so would like to double check final patch

Cool, thanks again @Campbell Barton (campbellbarton), I haven't had much time for Blender lately (using or programming) since it's summer and I have a real job... but I haven't forgotten about this, it's nice that it's going somewhere =)

Commandeering the rev as I had to update slightly the patch against latest master, also made some very basic style cleanup: mostly, floats should always be in the '0.0f' format, not '0' or '.125' etc., and avoid one-line if() conditions (we only have that when stashing a bunch of simple checks, else always better to put code of condition in next line).

Real patch review will follow.

Update against latest trunk, some minor style cleanup.

Globally, patch looks good to me.

In addition to the (rather picky!) inline comments, here are a few general remarks:

You forgot to add ED_object_add_mesh_props() call in grid op! ;)

When generating UVMap for non-capped cylinder, shouldn’t the side faces take the whole UV space?
Same for non-capped cone, shouldn’t cone itself take the whole space as well?

source/blender/bmesh/operators/bmo_primitive.c
520	shouldn’t this func be static? If static, please also rename it to `bm_mesh_calc_uvs_sphere_face`
521	With UVSphere or IcoSphere, len will never be > 4, no need to use alloca in this case imho, just simply use a float (*)[4] table (with a BLI_assert on f->len)? Or do you want this func to be usable with any kind of sphere-like geometry?
525	better to use zero_v3(v) later. But do this vec need to be initialized at all?
538	Conventions are to use post-increment in our C code (same for other loops below).
545	eeeh, a one-line if() I missed!
559	another one I missed, no spaces between brackets and numbers, and please use full float format (1.0f).
657–658	Why is this removed???
866–867	This can rather simply be `zero_v3(inv_mat[3])`
1037	That flag is already disabled here. ;)
1060	No need to set dx/dy in the default case imho, this one will never be reached anyway.

@Liam Mitchell (CommanderCorianderSalamander) please commandeer the diff again to make requested changes. :)

Liam Mitchell (CommanderCorianderSalamander) commandeered this revision.Jul 19 2014, 11:23 PM

Liam Mitchell (CommanderCorianderSalamander) edited reviewers, added: Bastien Montagne (mont29); removed: Liam Mitchell (CommanderCorianderSalamander).

Liam Mitchell (CommanderCorianderSalamander) added inline comments.

source/blender/bmesh/operators/bmo_primitive.c
520	It's declared static at the top of the file ;)
657–658	Just moved up a few lines so all the faces were made before the dissolve_faces call - it made calc_uvs simpler if it could assume it was capped with triangles (I think! it's been a while)

Updated diff with requested changes.

Ok, sorry for those stupid comments then. ;)

Patch looks good to me, I have a remaining question about whether we want uv-generation funcs to be public, or remain local, since we only use them in one place? But that’s more a question for Campbell I guess (design decision), let’s see what he thinks about it. :)

This revision is now accepted and ready to land.Jul 24 2014, 8:08 PM

Quick review:

UV calc functions shouldn't reference FACE_MARK, makes them less re-usable, instead take an const short oflag argument. As bmo_recalc_face_normals_array does.
BMO_elem_flag_disable(bm, f, FACE_MARK); Why do this? the caller may want to use the flag still (in fact it seems quite likely).
Adding a mesh adds a new UV layer every time check what UV unwrap does to avoid that.

source/blender/bmesh/operators/bmo_primitive.c
313	Why disable?
700	style: http://wiki.blender.org/index.php/Dev:Doc/Code_Style#Braces
896	Why disable?
source/blender/editors/object/object_add.c
307	Why Skip-Save? seems this is useful to remember.

This revision now requires changes to proceed.Jul 25 2014, 9:52 AM

sorry, but can't this make it into 2.72..? seems good for new users and helps on painting workflow

@lower case (lowercase), there are still concerns with the patch, best resolve these first and not worry about which release it gets into.

Fixed adding a new UV layer every time an object is added, moved responsibility for disabling flags out into the functions that enable them, little style cleanups, allowed generate UVs option to be saved, fixed a one-line if

The patch still seems half finished.

Functions need to accept operator-flag so they only uv-unwrap new geometry.
Some flag use seems odd (noted inline)

source/blender/bmesh/operators/bmo_primitive.c
291	Whats the purpose of disabling these flags?
572	This is applying to all faces, which will overwrite existing UV's if you add a new sphere while in edit-mode.

The disabling of the flags was how I chose to solve the problem of creating a new primitive in editmode (the problem that isn't handled properly for spheres) - ie. flagging only the faces that require UVs with FACE_MARK/FACE_NEW then disabling the flag after the call to BM_mesh_calc_uvs_*(). Do you think there's a better way of handling this @Campbell Barton (campbellbarton) or does that seem reasonable to you?

source/blender/bmesh/operators/bmo_primitive.c
291	If they stay enabled, those faces get unwrapped again next time BM_mesh_calc_uvs_grid is called on this mesh (ie. adding a second primitive, with unwrapping, to the same mesh as the first). Those flags were just enabled in this function directly above.
572	Yes - this is actually the problem I was trying to solve in the other primitives by enabling/disabling FACE_NEW. Forgot to deal with it here =(

@Liam Mitchell (CommanderCorianderSalamander)

This patch seems quite close but still needs some fixes regarding flag use.

If you don't have time for this could you let us know and someone else could finish it off.

source/blender/bmesh/operators/bmo_primitive.c
291	These flags are reset for each bmesh-operator call, you shouldn't have to initialize them. If you do - then it must be some bug in the operator flag stack.

This revision now requires changes to proceed.Feb 2 2015, 3:10 PM

lower case (lowercase) removed a subscriber: lower case (lowercase).May 1 2015, 2:08 AM

Time to finalize this!

Closed by commit rB3c6709a63c26: Fix T37879: Default UV generation for mesh primitives. (authored by Bastien Montagne (mont29)). · Explain WhyDec 5 2015, 12:53 AM

This revision was automatically updated to reflect the committed changes.

Default UV generation for mesh primitives
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1584

source/blender/bmesh/intern/bmesh_opdefines.c

source/blender/bmesh/intern/bmesh_operators.h

source/blender/bmesh/operators/bmo_primitive.c

source/blender/editors/include/ED_object.h

source/blender/editors/mesh/editmesh_add.c

source/blender/editors/object/object_add.c

Default UV generation for mesh primitivesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1584

source/blender/bmesh/intern/bmesh_opdefines.c

source/blender/bmesh/intern/bmesh_operators.h

source/blender/bmesh/operators/bmo_primitive.c

source/blender/editors/include/ED_object.h

source/blender/editors/mesh/editmesh_add.c

source/blender/editors/object/object_add.c

Default UV generation for mesh primitives
ClosedPublic
Actions

Revision Contents
Changeset List