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

Wed Aug 10 09:52:01 EDT 2011

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/d1695e22/attachment.html>