MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0028368Open CASCADE[OCCT] OCCT:Foundation Classespublic2017-01-18 11:282017-09-29 16:24
Reporterdbp 
Assigned Toapn 
PrioritynormalSeverityminor 
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version[OCCT] 7.2.0Fixed in Version[OCCT] 7.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
Attached Files

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

-  Notes
(0062739)
git (administrator)
2017-01-18 11:31

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
(0062740)
dbp (developer)
2017-01-18 11:32

Dear abv,

Could you please review the patch in branch CR28368?
(0062762)
abv (manager)
2017-01-18 16:54

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.
(0062767)
dbp (developer)
2017-01-18 17:11

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?
(0062769)
abv (manager)
2017-01-18 17:31

Reviewed, please test.

I have created separate issue 0028372 to treat remarks related to design of BVH_Tree class.
(0062804)
apv (tester)
2017-01-19 15:53

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%]
(0063617)
git (administrator)
2017-02-08 11:36

Branch CR28368 has been deleted by kgv.

SHA-1: e7e7c61b93e1345521fde1de8791d5760585285a

- Related Changesets
occt: master f63101c9
Timestamp: 2017-01-18 08:30:08
Author: dbp
Committer: apn
Details ] Diff ]
0028368: TKMath, BVH -- Fix invalid tree height in QBVH
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 View Revisions
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 user533 Fixed in Version => 7.2.0
2017-09-29 16:24 user533 Status verified => closed


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker