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 |
| Reporter | Assigned To | apn | |||
| Priority | normal | Severity | minor | ||
| Status | closed | Resolution | fixed | ||
| 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. SHA-1: e7e7c61b93e1345521fde1de8791d5760585285a Detailed log of new commits: Author: dbp Date: Wed Jan 18 11:30:08 2017 +0300 0028368: TKMath, BVH -- Fix invalid tree height in QBVH |
|
|
Dear abv, 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. |
|
|
Dear abv, 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. |
|
|
Dear BugMaster, Branch CR28368 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested. SHA-1: e7e7c61b93e1345521fde1de8791d5760585285a Number of compiler warnings: occt component: Linux: 0 (0 on master) Windows: 0 (0 on master) MasOS: 0 (0 on master) products component: Linux: 63 Windows: 0 MacOS: 1136 Regressions/Differences: Not detected Testing cases: Not needed 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. SHA-1: e7e7c61b93e1345521fde1de8791d5760585285a |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2017-01-18 11:28 |
|
New Issue | |
| 2017-01-18 11:28 |
|
Assigned To | => dbp |
| 2017-01-18 11:31 | git | Note Added: 0062739 | |
| 2017-01-18 11:32 |
|
Note Added: 0062740 | |
| 2017-01-18 11:32 |
|
Assigned To | dbp => abv |
| 2017-01-18 11:32 |
|
Status | new => resolved |
| 2017-01-18 11:32 |
|
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 |
|
Note Added: 0062762 | |
| 2017-01-18 17:11 |
|
Note Added: 0062767 | |
| 2017-01-18 17:18 |
|
Assigned To | abv => dbp |
| 2017-01-18 17:18 |
|
Status | resolved => feedback |
| 2017-01-18 17:30 |
|
Relationship added | related to 0028372 |
| 2017-01-18 17:31 |
|
Note Added: 0062769 | |
| 2017-01-18 17:31 |
|
Assigned To | dbp => bugmaster |
| 2017-01-18 17:31 |
|
Status | feedback => reviewed |
| 2017-01-18 17:53 |
|
Test case number | => Not needed |
| 2017-01-18 17:53 |
|
Assigned To | bugmaster => apv |
| 2017-01-19 15:53 |
|
Note Added: 0062804 | |
| 2017-01-19 15:53 |
|
Assigned To | apv => bugmaster |
| 2017-01-19 15:53 |
|
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 | |
| 2017-09-29 16:21 |
|
Fixed in Version | => 7.2.0 |
| 2017-09-29 16:24 |
|
Status | verified => closed |
