[vtk-developers] corrected-patch: Cell Tree Locator Update
Tharindu De Silva
tsameera1 at gmail.com
Thu Aug 11 13:03:57 EDT 2011
Hi Andy,
I didn't have the permissions earlier. I submitted the online
Kitware password form last night. I am waiting a response for that.
Thanks
Tharindu
On Wed, Aug 10, 2011 at 7:46 PM, Andy Bauer <andy.bauer at kitware.com> wrote:
> Hi Tharindu,
>
> Did you get merge permissions for the main vtk repo (
> http://www.vtk.org/Wiki/VTK/Git#Pushing)? If not that may be it. If
> that's set then you should just be able to use the aliases "git stage-push"
> then "git stage-merge" as long as you're on the cell tree locator branch you
> want to merge in. If neither of them solve your problem you may have to
> give more details about the error message.
>
> Andy
>
>
> On Wed, Aug 10, 2011 at 7:10 PM, Tharindu De Silva <tsameera1 at gmail.com>wrote:
>
>> 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/20110811/a2fcd253/attachment.html>
More information about the vtk-developers
mailing list