View Issue Details

IDProjectCategoryView StatusLast Update
0028368Open CASCADEOCCT:Foundation Classespublic2017-09-29 16:24
ReporterdbpAssigned Toapn  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Target Version7.2.0Fixed in Version7.2.0 
Summary0028368: TKMath, BVH - Fix invalid tree height in QBVH
DescriptionCollapseToQuadTree() has an issue leading to incorrect calculation of QBVH tree height.
Steps To ReproduceN/A
TagsNo tags attached.
Test case numberNot needed

Relationships

related to 0027590 closedbugmaster Visualization, Ray Tracing - port to quad BVH trees (QBVH) 
related to 0028372 assignedabv Foundation Classes - BVH_Tree class refactoring 

Activities

git

2017-01-18 11:31

administrator   ~0062739

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

dbp

2017-01-18 11:32

developer   ~0062740

Dear abv,

Could you please review the patch in branch CR28368?

abv

2017-01-18 16:54

manager   ~0062762

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.

dbp

2017-01-18 17:11

developer   ~0062767

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?

abv

2017-01-18 17:31

manager   ~0062769

Reviewed, please test.

I have created separate issue 0028372 to treat remarks related to design of BVH_Tree class.

apv

2017-01-19 15:53

tester   ~0062804

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%]

git

2017-02-08 11:36

administrator   ~0063617

Branch CR28368 has been deleted by kgv.

SHA-1: e7e7c61b93e1345521fde1de8791d5760585285a

Related Changesets

occt: master f63101c9

2017-01-18 08:30:08

dbp


Committer: apn Details Diff
0028368: TKMath, BVH -- Fix invalid tree height in QBVH Affected Issues
0028368
mod - src/BVH/BVH_BinaryTree.lxx Diff File

Issue History

Date Modified Username Field Change
2017-01-18 11:28 dbp New Issue
2017-01-18 11:28 dbp Assigned To => dbp
2017-01-18 11:31 git Note Added: 0062739
2017-01-18 11:32 dbp Note Added: 0062740
2017-01-18 11:32 dbp Assigned To dbp => abv
2017-01-18 11:32 dbp Status new => resolved
2017-01-18 11:32 dbp 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
2017-01-18 17:11 dbp 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
2017-01-18 17:53 apv Test case number => Not needed
2017-01-18 17:53 apv Assigned To bugmaster => apv
2017-01-19 15:53 apv Note Added: 0062804
2017-01-19 15:53 apv Assigned To apv => bugmaster
2017-01-19 15:53 apv 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 aiv Fixed in Version => 7.2.0
2017-09-29 16:24 aiv Status verified => closed