[vtk-developers] corrected-patch: Cell Tree Locator Update

Wed Aug 10 19:10:29 EDT 2011

Hi Andy,

      I got a permission denied error when I tried to merge that from git
bash.  I tried the ssh git at vtk.org stage VTK merge CellTreeLocator command.
 Do I have the permission to do this or is there an alternative way.

Thanks,
Tharindu

On Wed, Aug 10, 2011 at 9:52 AM, Andy Bauer <andy.bauer at kitware.com> wrote:

> I checked out the patch and I'm fine with it.  I also agree with John that
> it's ready to be merged into VTK.  Tharindu, Jeff tells me 8/15 is your last
> official day so I'd say try to add the patch and merge this into the main
> repo ASAP so that you can monitor the dashboards for a day or two and make
> appropriate fixes.  Then if time allows I'd recommend:
>
> 1) add in an example as I emailed about earlier
>
> 2) do some profiling of the locator to see if you can improve it's
> performance as the key to its use will be how fast it is
>
> 3) do some performance comparison tests with other cell locators and email
> the VTK developers and users list giving results.
>
> This may be too much to get done before 8/15 but I figured that if all this
> got done it would be a nice wrap-up for the good work that you've done as
> well as getting it used in other parts of VTK to improve performance.
>
> Thanks,
> Andy
>
>
> On Wed, Aug 10, 2011 at 6:20 AM, Biddiscombe, John A. <biddisco at cscs.ch>wrote:
>
>>  PS.****
>>
>> ** **
>>
>> I have been playing with the locator quite a bit, and if the kitware guys
>> are happy with the style, then I’d like to recommend it for merging into the
>> next release.****
>>
>> ** **
>>
>> Thanks****
>>
>> ** **
>>
>> JB****
>>
>> ** **
>>
>> ** **
>>
>> *From:* vtk-developers-bounces at vtk.org [mailto:
>> vtk-developers-bounces at vtk.org] *On Behalf Of *Biddiscombe, John A.
>> *Sent:* 10 August 2011 12:10
>>
>> *To:* Tharindu De Silva; Andy Bauer; VTK Developers
>> *Subject:* [vtk-developers] corrected-patch: Cell Tree Locator Update****
>>
>>  ** **
>>
>> This patch is the correct one. My apologies.****
>>
>> ** **
>>
>> JB****
>>
>> ** **
>>
>> ** **
>>
>> *From:* vtk-developers-bounces at vtk.org [mailto:
>> vtk-developers-bounces at vtk.org] *On Behalf Of *Biddiscombe, John A.
>> *Sent:* 10 August 2011 09:50
>> *To:* Tharindu De Silva; Andy Bauer; VTK Developers
>> *Subject:* Re: [vtk-developers] Cell Tree Locator Update****
>>
>> ** **
>>
>> The virtual call in the destructor was my fault. Sorry about that. I am
>> aware of the badness of virtual calls in constructors and destructors. I
>> think it’s safe in this case, because it just frees some pointers and sets
>> them to null, the base class does the same, but doesn’t know about one
>> pointer, so worst case scenario is a memory leak (but it’s hard to see this
>> happening with your change).****
>>
>> ** **
>>
>> The class does not honour the Max cells per node variable, it declares
>> it’s own one and ignore the one inherited from abstract cell locator. the
>> patch attached fixes that and also cleans up a doc entry which was nonsense.
>> ****
>>
>> ** **
>>
>> JB****
>>
>> Apologies I can’t use Gerrit due to inability to get an openID****
>>
>> ** **
>>
>> *From:* Tharindu De Silva [mailto:tsameera1 at gmail.com]
>> *Sent:* 09 August 2011 23:44
>> *To:* Andy Bauer; VTK Developers; Biddiscombe, John A.
>> *Subject:* Re: [vtk-developers] Cell Tree Locator Update****
>>
>> ** **
>>
>> ** **
>>
>> On Tue, Aug 9, 2011 at 5:43 PM, Tharindu De Silva <tsameera1 at gmail.com>
>> wrote:****
>>
>> Hi Andy,****
>>
>> ** **
>>
>>           Thank you for the comments.  Sorry I didn't notice the trailing
>> white spaces.  ****
>>
>>         ****
>>
>>           By 'example' i meant the test,  I will go through the wiki page
>> and try to make an example.  ****
>>
>> ** **
>>
>> Thanks,****
>>
>> ** **
>>
>> Tharindu****
>>
>> On Tue, Aug 9, 2011 at 4:44 PM, Andy Bauer <andy.bauer at kitware.com>
>> wrote:****
>>
>> Hi Tharindu,
>>
>> I cleaned up the code a bit doing things like getting rid of trailing
>> whitespace (gerrit will highlight this as red to warn about it), bracketed
>> some if/else conditions, and added a std::vector to take care of some
>> dynamic memory allocation.  I also got rid of the compile warnings in the
>> test.  These changes are at:
>> http://review.source.kitware.com/#change,2418
>>
>> One thing to note is that I made it more apparent that a virtual function
>> was getting called in the destructor.  In general this is bad practice (see
>> http://www.artima.com/cppsource/nevercall.html).
>>
>> I was able to run the test successfully.  In your previous email you
>> mentioned an example.  Did you mean test?  I couldn't find anything called
>> CellTreeLocator.cxx except for the test.  If you wanted to add in a
>> different example, please check out: http://www.vtk.org/Wiki/VTK/Examples.
>> This is not required but I'd encourage you to do this to help other people
>> better understand and utilize your code.
>>
>> Andy****
>>
>> ** **
>>
>> On Tue, Aug 9, 2011 at 11:23 AM, Tharindu De Silva <tsameera1 at gmail.com>
>> wrote:****
>>
>> Hi,****
>>
>> ** **
>>
>>   I revert the renaming of internal variables to their original names and
>> made a note on the code that they were derived from avtCellLocatorBIH class
>> in VisIT, so that the people familiar with that code can make improvements.
>>  The current code can be found in
>> http://review.source.kitware.com/#change,2413.****
>>
>> ** **
>>
>>  There is an example named CellTreeLocator.cxx at Filtering/Examples/Cxx.
>>  Please let me know if you encounter any bugs in running this.****
>>
>> ** **
>>
>> Thank you.****
>>
>>  ****
>>
>> On Tue, Aug 2, 2011 at 5:08 PM, Biddiscombe, John A. <biddisco at cscs.ch>
>> wrote:****
>>
>> I’m not sure that renaming all the internal variables from their original
>> names was a good idea.****
>>
>> The original code came from the visit avtXXX class and there’s a strong
>> possibility that the visit developers will make changes/improvements similar
>> to the intersectBox type methods that I’m adding. ****
>>
>>  ****
>>
>> Renaming the internals serves no purpose other than to make it harder to
>> sync our version with theirs.****
>>
>>  ****
>>
>> I am aware that I have radical opinions about the dreadful kitware code
>> style but for me this is an unnecessary barrier to future collaboration.*
>> ***
>>
>>  ****
>>
>> JB****
>>
>>  ****
>>
>> *From:* Tharindu De Silva [mailto:tsameera1 at gmail.com]
>> *Sent:* 02 August 2011 18:18
>> *To:* Biddiscombe, John A.; Andy Bauer; VTK Developers
>> *Cc:* trshash at gmail.com****
>>
>>
>> *Subject:* Re: [vtk-developers] Cell Tree Locator Update****
>>
>>  ****
>>
>> Hi,****
>>
>>  ****
>>
>>          I did some formatting changes to the code according to vtk
>> guidelines and added the patch given by John.  The code can be found in
>> http://review.source.kitware.com/#change,2362.****
>>
>>  ****
>>
>> Some formatting still needs to be done.****
>>
>>  ****
>>
>> Helper classes, vtkCellTree and vtkCellTreeNode, are made public in the
>> current version to make them accessible by sub-classing to test for
>> additional functions implemented in vtkModifiedBSPTree. This can be made
>> protected after all the functions are available in vtkCellTreeLocator.***
>> *
>>
>>  ****
>>
>> The standard template library functions are used in this class that might
>> need to be changed as well.****
>>
>>  ****
>>
>> Please let me know if you come across any other bugs or if there are
>>  improvements that can be made to this code.****
>>
>>  ****
>>
>> Thank you,****
>>
>>  ****
>>
>> Tharindu ****
>>
>>  ****
>>
>> On Fri, Jul 29, 2011 at 4:02 PM, Biddiscombe, John A. <biddisco at cscs.ch>
>> wrote:****
>>
>> I’ve fixed the crash caused by a numeric overflow. some compilers might
>> not mind and not cause the segfault – I believe the intent was to allow the
>> std:::numeric_limits max() to cause a large volume result which would
>> exclude the bucket from consideration.****
>>
>>  ****
>>
>> I’d added a check to stop it. Might be worth adding a few other tests as
>> this class is new and we have little experience with  it. I’ll add one
>> myself I think.****
>>
>>  ****
>>
>> patch attached****
>>
>>  ****
>>
>> JB****
>>
>>  ****
>>
>>
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Follow this link to subscribe/unsubscribe:
>> http://www.vtk.org/mailman/listinfo/vtk-developers****
>>
>>  ****
>>
>> ** **
>>
>> ** **
>>
>> ** **
>>
>> ** **
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20110810/7ea587f3/attachment.html>