Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(13)

Issue 6114060: Convex hull bmesh operator (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years ago by nicholasbishop
Modified:
5 years ago
Reviewers:
ideasman42, bf-codereview
Base URL:
https://svn.blender.org/svnroot/bf-blender/trunk/blender/
Visibility:
Public.

Description

As part of my skin modifier work I added a convex hull operation. I think this can be useful as a general modeling tool, this patch contains the BMesh operator and corresponding MESH_OT_convex_hull. Example: http://nicholasbishop.net/wordpress/wp-content/uploads/2012/04/hull.gif

Patch Set 1 #

Total comments: 3

Patch Set 2 : Update 1 #

Total comments: 4

Patch Set 3 : Update 2 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+872 lines, -9 lines) Patch
source/blender/blenlib/BLI_ghash.h View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
source/blender/bmesh/CMakeLists.txt View 1 2 1 chunk +1 line, -0 lines 0 comments Download
source/blender/bmesh/intern/bmesh_error.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
source/blender/bmesh/intern/bmesh_opdefines.c View 1 2 2 chunks +32 lines, -0 lines 0 comments Download
source/blender/bmesh/intern/bmesh_operators_private.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
source/blender/bmesh/operators/bmo_hull.c View 1 2 1 chunk +741 lines, -0 lines 0 comments Download
source/blender/editors/mesh/editmesh_tools.c View 1 2 3 chunks +87 lines, -9 lines 0 comments Download
source/blender/editors/mesh/mesh_intern.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
source/blender/editors/mesh/mesh_ops.c View 1 2 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 9
nicholasbishop
5 years ago (2012-04-26 16:21:46 UTC) #1
ideasman42
Tried on this file and it gives a problem at the top monkeys head. The ...
5 years ago (2012-04-27 03:23:59 UTC) #2
nicholasbishop
Thanks for reviewing :) New patch attached, comments below: > Tried on this file and ...
5 years ago (2012-04-28 03:24:26 UTC) #3
ideasman42
Some more feedback, If you want to copy loop data from adjacent faces call this ...
5 years ago (2012-04-28 10:59:34 UTC) #4
nicholasbishop
Thanks, new patch uploaded. > If you want to copy loop data from adjacent faces ...
5 years ago (2012-04-28 20:05:01 UTC) #5
ideasman42
LGTM, one more general comment is that I think having the operator delete the holes ...
5 years ago (2012-04-28 20:36:46 UTC) #6
nicholasbishop
On 2012/04/28 20:36:46, ideasman42 wrote: > LGTM, > > one more general comment is that ...
5 years ago (2012-04-28 21:18:42 UTC) #7
ideasman42
On 2012/04/28 21:18:42, nicholasbishop wrote: > On 2012/04/28 20:36:46, ideasman42 wrote: > > LGTM, > ...
5 years ago (2012-04-29 06:13:32 UTC) #8
nicholasbishop
5 years ago (2012-04-29 16:11:35 UTC) #9
On 2012/04/29 06:13:32, ideasman42 wrote:
> On 2012/04/28 21:18:42, nicholasbishop wrote:
> > On 2012/04/28 20:36:46, ideasman42 wrote:
> > > LGTM,
> > > 
> > > one more general comment is that I think having the operator delete the
> holes
> > > its self might not be so good.
> > > 
> > > Instead the blender operator could do this on faces tagged as original. -
> This
> > > isnt really bad, just think it can get a bit messy if a lot of operators
> call
> > > eachother because of functionality users might want. rather have them low
> > level
> > > and the callers can mix together the operators that make sense in some
> > context.
> > > In this case the caller is the wmOperator
> > 
> > Before, I was doing deletion in the wmOperator, but the code got a bit more
> > complex than just deleting original faces -- all the code in
> > hull_delete_unused() and hull_make_holes(). Agree it gets trickier to
combine
> > operators now, but on the other hand  the hull operator has flags to disable
> > these operations too.
> > 
> > So, not sure which option is best:
> > a) In the hull bmop, find elements to be deleted and tag them in an output
> slot,
> > then actually delete in the wm op
> > b) Move both the find-deleteable-elements and deletion to the wm op
> > c) Leave both in the bmop and let other operators disable with slot booleans
> 
> Leave this up to you, but some things to consider.
> 
> I Don't think the way its doing it now is horrible, off hand (a) looks most in
> keeping with bmop design, but if for some reasons there are complications then
> suggest stick with what you have (c).
> 
> b) may be more trouble then its worth, but if it could be done cleanly then
I'd
> consider it - rip tool does this for eg.

Implemented option (a), committed to r46080 and r46081.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted