Re: [ggt-devel] Submitting new features and some proposals

[Patrick H. <pat...@gm...>]

On Mar 10, 2011, at 9:23 AM, Sebastian Messerschmidt wrote:

> Am 10.03.2011 16:12, schrieb Patrick Hartling:
>> On Mar 10, 2011, at 8:27 AM, Sebastian Messerschmidt wrote:
>> 
>>> Todd,
>>>> Sebastian,
>>>> 
>>>> I'm interested in seeing GMTL maintained&   updated as well, and it
>>>> sounds like you have some valuable additions.  I'll let the project
>>>> admins chime in as to how you can submit patches.
>>>> 
>>>> I don't think the boost-related code should be part of GMTL, but it
>>>> sounds like you have the foundation for maybe a gmtl-boost header library?
>>> There is already boost related code in the Misc/MatrixConvert.h . IIRC
>>> it is invoked only if you include certain header files explicitly.
>>> For my includes you would have to include the gmtl_serializers.h only if
>>> you need to serialize via boost.
>>> I think that'd be a viable solution, because gmtl is header only, so it
>>> won't harm to deploy a number of extra headers which not mandatory to use.
>> I agree.
>> 
>>>> 10/2011 5:25 AM, Sebastian Messerschmidt wrote:
>>>>> Hi,
>>>>> 
>>>>> it's been a while since I've heard anything from this list.
>>>>> If it is still active I'd have to requests/questions.
>>>>> 
>>>>> 1. I've extended some of stuff in the gmtl library, and like to submit
>>>>> them for a review.
>>>>> Namely I've added some new functions:
>>>>> 
>>>>> 1.1 A trivial sphere sphere intersection test (I didn't find one)
>>>>> 
>>>>> bool intersect(const Sphere<DATA_TYPE>&    sphere1, const
>>>>> Sphere<DATA_TYPE>&    sphere2)
>>>>> 
>>>>> 1.2 double sided intersection function for Triangles and LineSegments
>>>>> (my last contribution only contained the ray intersection)
>>>>> 
>>>>> template<class DATA_TYPE>
>>>>>      bool intersectDoubleSided( const Tri<DATA_TYPE>&    tri, const
>>>>> LineSeg<DATA_TYPE>&    lineseg,
>>>>>          DATA_TYPE&    u, DATA_TYPE&    v, DATA_TYPE&    t )
>>>>> 
>>>>> 1.3 TriOps: Closest point to triangle
>>>>> 
>>>>> template<    class DATA_TYPE>
>>>>> Point<DATA_TYPE, 3>    findNearestPt( const Tri<DATA_TYPE>&    tri,
>>>>>                                      const Point<DATA_TYPE, 3>&    pt )
>>>>> 
>>>>> 
>>>>> 
>>>>> 2. Furthermore I'd like to discuss some topics, that might be beneficial
>>>>> to the gmtl library.
>>>>> 
>>>>> 2.1 Issues with the epsilon values used inside some of the functions e.g.
>>>>> template<typename DEST_TYPE, typename DATA_TYPE>
>>>>>      inline DEST_TYPE&    setRot( DEST_TYPE&    result, const Vec<DATA_TYPE,
>>>>> 3>&    from, const Vec<DATA_TYPE, 3>&    to )
>>>>> 
>>>>> I've had to change the epsilon here, else the from and to vectors where
>>>>> considered to be close enough for long distance camera aiming.
>>>>> To solve this problem in general, I also have proposed a generalized fix
>>>>> on the bug list. It involves templated type dependent epsilon usage
>>>>> 
>>>>> 2.2 There are still issues with inconsistent interfaces for some of the
>>>>> intersect functions:
>>>>> 
>>>>> E.g.
>>>>> 
>>>>>    inline bool intersect( const Sphere<T>&    sphere, const Ray<T>&    ray,
>>>>> int&    numhits, T&    t0, T&    t1 )
>>>>> 
>>>>>      inline bool intersectVolume( const Sphere<T>&    sphere, const
>>>>> LineSeg<T>&    ray, int&    numhits, T&    t0, T&    t1 )
>>>>> 
>>>>>      inline bool intersectVolume( const Sphere<T>&    sphere, const Ray<T>&
>>>>> ray,  int&    numhits, T&    t0, T&    t1 )
>>>>> 
>>>>>      inline bool intersectVolume( const Sphere<T>&    sphere, const Ray<T>&
>>>>> ray,  int&    numhits, T&    t0, T&    t1 )
>>>>> 
>>>>> Those functions are all taking integers for the numhits ref. Other
>>>>> intersect functions take unsigned int. I think this is fairly
>>>>> inconsistent, and bugs me.
>>>>> The reason I would rather set them all to unsigned int is simple: I have
>>>>> some template functions that unify and wrap those functions. Right now I
>>>>> cannot use them for all intersect functions on the trunk-rev,
>>>>> since type inference will not allow for int/unsigned int mixtures.
>>>>> 
>>>>> 2.3. There has been a submission of mine ( like ages ago) concerning the
>>>>> Box/LineSeg intersection test.
>>>>> In my eyes, the early out test should be different to encompass
>>>>> LineSegments completely behind the box:
>>>>> 
>>>>> Instead of:
>>>>>       if (tIn<    0.0&&    tOut>    1.0)
>>>>> I'd rather advocate for:
>>>>>       if (tIn<    0.0&&    tOut>    DATA_TYPE(1)|| tIn>    DATA_TYPE(1)&&    tOut>
>>>>> DATA_TYPE(1))
>>>>> 
>>>>> 
>>>>> 2.4. I had to add private assignment operators in the VecBase and
>>>>> VecBinaryExpr classes to satisfy my compiler. These are trivial changes,
>>>>> but I'd like to see them reviewed and added.
>>>>> 
>>>>> 
>>>>> 2.5 I've experimented with Metaprogramming for Vector, Matrix
>>>>> constructors (eliminating the nested loop in the matrix and get it
>>>>> replaced to "fload").
>>>>> 
>>>>> 2.6. I've implemented non-intrusive boost::serialization functions for
>>>>> VecBase, Tri, Matrix, Quat, AABox and Sphere. If anyone is interested.
>>>>> 
>>>>> 2.7 boost::hash based hash functions for some gmtl types (Matrix and
>>>>> VecBase)
>>>>> 
>>>>> So how do I submit my proposals? Splitting this stuff up into smaller
>>>>> chunks shouldn't be a problem.
>>>>> I'd be happy to hear your opinions.
>> These all sound like great improvements. If you could break things up into multiple patches, that would help me review them. The GMTL issue tracker is pretty far off my radar, so if you have submitted patches through it that have gone unnoticed, I apologize for that. Submitting them through this mailing list makes it easier for me to keep track of things.
> Okay, the remaining question is the format of the patches. Do you want 
> diffs, complete files, svn patches?

Unified or context diffs work best for me.

 -Patrick


--
Patrick L. Hartling
Senior Software Engineer, Priority 5
http://www.priority5.com/