itaps-parallel [Fwd: **** iMesh.h and iBase.h changes ****]

[Jason Kraftcheck]

Ting Xie wrote:
> Hi all,
> 
> Our (RPI team) concerns on the iMesh.h and iBase.h changes are listed as
> follows. Please let us know if you have any questions. Thanks.
> 
> 
> --------------------------------------------------
> 
> We sort the item number in Jason's proposal in the order of priority. The
> function with the higher priority means that we have more confusion and it
>  might need more effort in the implementation.
> 
> 
> 1. Item #6
> 
>  -------------------------------------------
>  Replace two old functions:
>     FUNC_1 iMesh_getAllVtxCoords
>     FUNC_2 iMesh_getVtxCoordIndex
>  with one new function:
>     FUNC_3 iMesh_getAdjEntIndices
> 
>  -------------------------------------------
>  Functionality of these functions (abstracted from iMesh user-guide and
>  Jason's proposal):
> 
>    * FUNC_1 Gets the coordinates of the vertices contained in the entity set
>   as an array of doubles in the order specified by the user.
> 
>    * FUNC_2 Returns the indices of the vertices that define all entities of
>  a given type or topology in the mesh or entity set.
> 
>    * FUNC_3 Returns the indices of the adjacent entities of specified type,
>  for all  entities of a given type or topology in the mesh or entity set.
> 
>  -------------------------------------------
>  OUR CONCERNS:
> 
>   *** 1.1 FUNC_3 does not return the vertex coordinates, i.e., it can not
> replace FUNC_1.
> 
>    To get the coordinates for vertex array returned from FUNC_3, we still
> have to call iMesh_getVtxArrCoords function. This was already indicated
> in the example in Jason's proposal.
> 
>    >>> The same code in the new API will be:
>    >>>   iMesh_getAdjEntIndices( instance, root_set, ......
>    >>>   iMesh_getVtxArrCoords( instance, .....
> 
> 
>    But iMesh_getVtxCoords can not replace FUNC_1 either. Because it only
> gets the coordinates for an array of vertices, which is not
> entity_set_handle. To get the coordinates of all the vertices in the
> entity_set through iMesh_getVtxCoords, we have to call
> iMesh_getEntities first, which needs extra work (iterate all vertices
> in the entity_set).
> 

Is this a problem?  If so, why?


>    One good thing: if we call iMesh_getVtxArrCoords, and use vertex array
> returned from FUNC_3 as the input, the problem size is reduced.
> 
> 
>   *** 1.2 FUNC_3 can return the indices for adjacent entities of any type,
> not only verices.
> 
>    This increases the flexibility of FUNC_3. But do we need that? Does the
> adjacency index important for other topological entity type? Or most of
> the time the applications just want to obtain the adjacent entities. If
> that is the case, we can use iMesh_getAdjEntities instead of FUNC_3.
> 

Reducing the functionality by allowing it to only return vertex handles does
little to reduce the complexity of the API.  And it seems unlikely that it
would dramatically reduce the complexity of the implementation.  So why
limit the functionality?

And yes, the original two functions can be removed without adding any new
functions.  The same result can be accomplished using other API functions.
However, this type of functionality seems to be expected by some
applications and convenient for others.

As for an example of using iMesh_getAdjEntIndices to query for a type other
that vertices, consider writing a file containing elements of type
iMesh_POLYHEDRON.

>   Even if we want to get the adjacency indices, the indicies defined in
> those functions (both old version and new verion) are still not clear.
> Are
>   these functions based on the assumption that each mesh entity,
> particular vertex, has an unique integer id. This assumption might not
> be true or right, especially in larger mesh cases.
> 

No such assumption is made.  I apologize if you find the documentation
unclear.  I reviewed it and could find nothing that I thought indicated that
IDs were being assigned to vertices, so I'll need some help in correcting
this deficiency.

> 
>    *** 1.3 Jason's proposal used a simple hex mesh to claim the advantages
> of FUNC_3, but the real applications can be much more complex,
> especially for mixed topological mesh and non-manifold meshes.
> Considering all kinds of situations added from the flexibility, the
> implementation of FUNC_3 will be much more complex.
> 

I do not agree that the common case for using these functions will involve
more complex meshes.  But regardless of that, I do not see why
iMesh_getAdjEntIndices (FUNC_3) would need to be any more complex.  An
application might also need to call iMesh_getEntArrTopo.  Can you provide
examples of the functionality you think this function is missing?

> 
>   *** 1.4 Compare the argument lists of FUNC_3 and iMesh_getAdjEntities:
> FUNC_3 returns both entity_handles of requested type/topo and
> adj_entity_handles of specified type, while FUNC_2 does not.
> 
>    The extra outputs eliminate a lot of unnecessary extra work within the
> implementations. But this might be a problem if application does not
> want to get entity_handles and adj_entity_handles info. And the outputs
> will take a lot of memory, especially when the two arrays are huge.
> 

If the application does not want adj_entity_handles, then it should call
iMesh_getEntities instead.  If by this you are repeating your concern from
your "1.1" point above, then yes I agree that an additional array of vertex
handles must be populated.  However, I still do not see this as an issue.
While the vertex handle array will be large for a large mesh, it will be
small relative to the other arrays passed back form this function
(coordinates, indices, etc.)  And the target applications for this
functionality will not be calling these functions often (e.g. once at
startup.)  And there are too many cases where the application will need the
vertex handles anyway (e.g. tag data, modifying vertex coordinates, etc.)

If you're concerned about allocating extra arrays, I'd be much more
concerned about the "in_entity_set" arguments.  The functions returning them
probably get called much more frequently and the number of cases where the
application wants the additional data is a much smaller fraction.  Further,
the cost of pupulating it is often much more.  The fact that
iMesh_getAllVtxCoords had such an argument and the new functions do not is
in my opinion more than sufficient to offset the cost of creating and
populating the extra vertex handle array.


> 
>   *** 1.5 FUNC_1 and FUNC_2 are coupled together. The coordinate index
> from FUNC_2 depends on the order of the vertex coordinates being
> returned from FUNC_1 (indicated in iMesh user-guide).
> 
>    One reason to introduce FUNC_3 is to eliminate the coupling. But in
> fact, the index order from FUNC_3 should be consistent with the entity
> order returned from iMesh_getEntities.
> 

Why?

> 
> 
>   *** 1.6 entity_topologies argument is not provided in FUNC_3, as that in
> FUNC_2.
> 
>   This is a minor problem. And yes, we can use iMesh_getEntArrTopo to get
> the info, as Jason mentioned.
> 
> ---------------------------------------------
> 
> 
> 2. Item #4
> 
>     It is not clear about what is the definition about vertex geometric
> dimension.
> 
>     For example, a surface mesh which only has mesh faces, edges and
> vertices, the coordinates of the vertices can have (x, y, z) (curved
> surfaces). In such case, what's right vertex dimension size, 2 or 3?
> 

This has already been defined in the spec for the existing
getGeometricDimension function.  And it is implied by the "geometric" part
of the name.  The *geometric* dimension of such a mesh is 3.
The topological dimension for the mesh you define is 2.

> 3. Item #8
> 
>       It is not clear about how to replace iBase_EntityHandle with
> iBase_EntitySetHandle where appropriate.
> 

For those functions for which the passed handle is a handle for an entity
set, the type of said argument should be iBase_EntitySetHandle.  In some
cases it is incorrectly specified as iBase_EntityHandle.

> 4. Item #5
> 
>     This is a little confused. Yes, the value of the entity_set_handle
> (the pointer itself) is not changed. But the content of the entity_set
> (the value it points to) is changed (because of the remove/add
> operation).
> 

The const-ness of the argument only guarantees that the function will not
change the value of the entity_set_handle (the pointer *if* it is a
pointer.)  Specifying it as const or not const does not say anything about
what is done contents of the set.  Your misunderstanding is with the C
language, but I'll try to explain it anyway.  Let's assume that the handles
are pointers (as I think that is what FMDB does).  If I have this:
  typedef void* handle_type;
then this:
  const handle_type h;
is equivalent to this:
  void* const handle_type h; // the memory address in handle_type cannot be
                             // changed
not this:
  const void* handle_type h; // the thing pointed to cannot be changed.

To specify what you want using typedefs in C, it would be necessary to have
an additional typedef:
  typedef const void* const_handle_type;

However, doing that would be a bad idea for other reasons.  The most
important being that it completely contradicts the whole idea of opaque
handles as it only makes sense for pointers.  If the handle is an integer
index into some array of entity sets, then there is no use of const that can
possibly prohibit the modification of the referenced set by the application.

If the handle type is opaque, then there just isn't a way to specify that
the set contents cannot change using C syntax.  Declaring the arguments in
these functions non-const in a "we wish we could indicate that in C and are
going to redefine 'const' to mean that" way will just confuse application
developers.  The implementation does not change the value of the pointer,
putting const before the argument guarantees this to the application, and
therefore not putting it there implicitly declares an intention to modify
the handle value.

> 
> 5. Item #7
> 
>      New function: iMesh_isEntArrContained, raised from Item #6. It is an
> array version for iMesh_isEntContained.
> 

And the issue with this is...?

- jason