View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0028368||Open CASCADE||OCCT:Foundation Classes||public||2017-01-18 11:28||2017-09-29 16:24|
|Target Version||7.2.0||Fixed in Version||7.2.0|
|Summary||0028368: TKMath, BVH - Fix invalid tree height in QBVH|
|Description||CollapseToQuadTree() has an issue leading to incorrect calculation of QBVH tree height.|
|Steps To Reproduce||N/A|
|Tags||No tags attached.|
|Test case number||Not needed|
Branch CR28368 has been created by dbp.
Detailed log of new commits:
Date: Wed Jan 18 11:30:08 2017 +0300
0028368: TKMath, BVH -- Fix invalid tree height in QBVH
Could you please review the patch in branch CR28368?
I have no remarks on the fix itself, however there are some general remarks on implementation of relevant BVH types:
1. Why structures BVH_BinaryTree (and BVH_QuadTree) and class BVH_TreeBase are defined not in the same-named headers but in BVH_Tree.hxx? This looks illogical. Note that in OCCT we have convention that a class should notmally be defined within the same-name header. Why not to follow this convention in this case?
2. The names like BVH_Tree and BVH_BinaryTree are suggesting that the latter is successor of the former. This is not the case. Instead, BVH_Tree is dummy that has two specializations, while BVH_BinaryTree and BVH_QuadTree are just tags used to trigger specializations. Why this complexity? It seems to be more straightforward just to define class BVH_Tree (or BVH_TreeBase) and two descendants, BVH_BinaryTree and BVH_QuadTree, providing relevant specializations.
The goal of this design was to provide two different and incompatible implementations for 2- and 4-ary trees, since these trees are very different in usage. 4-ary tree is always produced from 2-ary tree, and is read-only. Also, it was decided not to use virtual functions for performance reasons.
I agree that names for BVH_BinaryTree and BVH_QuadTree structures looks strange, and we should change it. For the rest, the implementation is quite natural from the point of template-based package.
Should we really redesign it now to follow point 1?
Reviewed, please test.
I have created separate issue 0028372 to treat remarks related to design of BVH_Tree class.
Branch CR28368 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
Number of compiler warnings:
Linux: 0 (0 on master)
Windows: 0 (0 on master)
MasOS: 0 (0 on master)
Testing on Linux:
Total MEMORY difference: 93120634 / 92913363 [+0.22%]
Total CPU difference: 21370.7800000002 / 21505.8700000003 [-0.63%]
Testing on Windows:
Total MEMORY difference: 58643092 / 58649007 [-0.01%]
Total CPU difference: 19444.042640498603 / 19749.742200098677 [-1.55%]
Branch CR28368 has been deleted by kgv.
||Assigned To||=> dbp|
|2017-01-18 11:31||git||Note Added: 0062739|
||Note Added: 0062740|
||Assigned To||dbp => abv|
||Status||new => resolved|
||Steps to Reproduce Updated|
|2017-01-18 11:56||kgv||Summary||TKMath, BVH -- Fix invalid tree height in QBVH => TKMath, BVH - Fix invalid tree height in QBVH|
|2017-01-18 14:05||kgv||Relationship added||related to 0027590|
|2017-01-18 16:54||abv||Note Added: 0062762|
||Note Added: 0062767|
|2017-01-18 17:18||abv||Assigned To||abv => dbp|
|2017-01-18 17:18||abv||Status||resolved => feedback|
|2017-01-18 17:30||abv||Relationship added||related to 0028372|
|2017-01-18 17:31||abv||Note Added: 0062769|
|2017-01-18 17:31||abv||Assigned To||dbp => bugmaster|
|2017-01-18 17:31||abv||Status||feedback => reviewed|
||Test case number||=> Not needed|
||Assigned To||bugmaster => apv|
||Note Added: 0062804|
||Assigned To||apv => bugmaster|
||Status||reviewed => tested|
|2017-01-20 16:12||apn||Changeset attached||=> occt master f63101c9|
|2017-01-20 16:12||apn||Assigned To||bugmaster => apn|
|2017-01-20 16:12||apn||Status||tested => verified|
|2017-01-20 16:12||apn||Resolution||open => fixed|
|2017-02-08 11:36||git||Note Added: 0063617|
||Fixed in Version||=> 7.2.0|
||Status||verified => closed|