[Bf-committers] Re: [Bf-blender-cvs] CVS commit:blender/source/blender/blenloader/intern readfile.cblender/source/blender/makesdna DNA_mesh_types.hDNA_meshdata_types.h blender/source/blender/src editmesh.ceditmesh_lib.c

[Geoffrey Bantle]

>I just noticed this code now, because of a crash
>report... I really think this should need a review 
>first, because; - Selections were already stored
>- This method, storing selections separately, makes
>the whole editmesh maintenance even more 
>complex as it was

Ton,

Sorry I didn't discuss this and my reasoning behind
this a bit with you first. I will explain now. I am
sorry if its a bit long-winded, but there are several
motivations behind my implementation of 'stored
selections' for EditMesh other than just 'adding
things for the sake of adding them'.

The idea for storing the 'order' in which
vertices/edges/faces were selected originally came
from my upgrade of the merge tools. Something that I
really wanted (and I knew that users did too), was the
ability to merge at the last vertex selected. This is
functionality that has real immediate benefits for the
user in terms of workflow enhancements. 

With this in mind I added a pointer in EditMesh to
keep track of the last vertex selected, called simply
'lastvert'. Later on a user suggested that it would be
logical to have the ability to merge at the first
selected vertex as well, so I added a 'firstvert'
pointer .  Now I understand that the 'firstvert'
pointer wasn't entirely consistent with blenders
'active object' paradigm, but it turned out to be such
a incredibly useful option that I kept it in. Once I
got it to play nice with the undo code, I felt I was
'done' with it...

Some things still  bugged me though. First of all its
wasn't really 'general' enough. It only worked for
vertices, but not faces and edges. It also seemed to
that it would be confusing for the end user that
leaving editmode and switching meshes would 'destroy'
the lastvert and firstvert information. However I felt
that for the time being it was 'good enough'.

Now later on I had to revisit the problem when I wrote
path select. Originally path select just selected the
shortest path between any two vertices. I was faced
with the problem of determining which two vertices to
use as the 'start' and 'end' of the path however.
Originally I solved this problem by only making it
possible to do path select when exactly two vertices
were selected. Unfortunately this was pretty useless
for the end user.

Looking at Silo's path select tool (which only works
part of the time BTW), it became obvious to me that in
order to make path select a truly useful feature you
would need to be able to allow the user to build their
path in a 'compound' fashion (as well as combine it
meaningfully with other selection tools). This meant
the user should be able to do something like this....

'Select A, then Select B, path select. Select C, path
select. Select D, path select...'

And so on. The problem with this was that the lastvert
pointer was not enough information because not only
did I need to know which was the *last* vertex
selected, but also what was the *next to last* vertex
selected. At first I considered abusing the undo stack
to find out this information, but then I realized
pretty quickly that it would be unwise to do so.

So I was faced with a problem, I had a tool that would
be incredibly useful for the end user if only I could
find out some information about the order in which the
user selected things. I also had the merge tools which
stored a small subset of this information already in
the 'firstvert' and 'lastvert' pointers. Furthermore I
had ideas for other tools that could use this sort of
information in useful ways. It seemed to me that
instead of creating several specific purpose systems
for keeping track of this info, it would be more
useful (and less organizational headache) to write a
general purpose facility.

So with this in mind I decided upon the following
things for this facility:

1: Only certain selections are significant (such as
     those the user makes themselves manually) 
     For instance It would not make sense for us    
     to store information for all the selections 
     made by 'select linked' or 'select all'. 
     Therefore any facility must allow storage of 
     selection order to be optional and up to 
     individual functions/tools.

2:  It has to work for verts/edges/faces

3: Keeping track of only the 'active' face/edge/vert
    was simply not enough for some tools like path 
    select and merge at first.

4: It has to work for multi-select mode

5: Given number 4 and 5, it should be implemented in
    such a way that all stored selections for 
     verts/edges/faces should be in a single ordered 
     list.

6: It should require the most minimal of changes to
    EditMesh.

Based upon these criteria I decided to add a ListBase
to EditMesh called 'selected' that would store
'EditSelections'. EditSelections would contain a void
pointer to either an EditVert, EditEdge or EditFace
and a variable reflecting its type.

In reality I don't see this as complicating editmesh
that much. If a function dosn't 'care' about selection
order, it can safely ignore stored EditSelections. If
the tool is designed in such a way that modifying the
selection state of of a vert/edge/face is considered
'significant' with respect to its 'selection order' it
can store the selection by simply calling
'EM_store_selection'.

The removal of stored selections is either done
explicitly with 'EM_remove_selection', or (more
commonly) is done implicitly when certain things
happen. For instance when a vert/edge/face is freed,
any EditSelection associated with it is also removed.
Also when selection modes are modified, any
EditSelections not relevant to the new selection mode
are deleted with 'EM_strip_selections'.

Things are a bit incomplete as of right now though;
currently EM_remove_selection is called explicitly
when the user deselects a vert/edge/face (in
mouse_mesh()). However this really should be done
whenever EM_select_face or EM_select_edge is called
with with its second argument equaling 'zero'. The
reason I havn't added this *yet* is because I would
like to first move all access of vertex selection to
an 'EM_select_vert' function to make everything
consistent. I already did a search through the source
of blender to find every place this would have to be
called, and if I can get the go-ahead from you I will
add it.

With regards to the modifications of Mesh DNA, I may
have been a bit overzealous there and messing with an
area I wasn't familiar enough with, but I had what I
felt were good reasons. The problem was that there was
no place to actually store EditSelections so that they
would be 'persistent' across editing sessions. In
other words it really bothered me that
entering/exiting
editmode and switching objects would destroy this data
and effectively 'break' features that depended on it.
In short I believe that in terms of user experience
the 'principle of least astonishment' should always be
followed.
 
So really the only solution that seemed viable to me
was storing this information in mesh DNA. On the other
hand it also seemed  overkill to have this information
written to file. Since I had been told in IRC by other
devs that sometimes DNA is used to store things during
'run-time only'  I thought it was a decent enough
solution.

If I wasn't clear on anything please let me know and I
will try to clarify. Once again I apologize that I
wasn't more clear with my intentions here and will
definitely be more mindful of this in the future. 

I look forward to hearing your thoughts on this
matter.


Cheers
Geoff Bantle

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com