Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: speed up point-object assignment with a bounding volume hierarchy #2270

Conversation

haakonflatval-cognite
Copy link
Contributor

@haakonflatval-cognite haakonflatval-cognite commented Jul 7, 2022

Description

Note: This PR is superseded by #2327 and will likely be deleted.

Speeds up the point-object matching process by using a Bounding Volume Hierarchy (BVH). It improves the loading/ point-object matching time for the root sector of the point cloud from ~35s to 1-2s.

Further suggestions for performance improvements are given in a JIRA issue.

This PR also fixes an issue where box matrices from the backend where not interpreted in row-major order as they should.

This PR currently targets the branch for workers in libraries, which it depends on. It should eventually target and be merged into the point cloud feature branch.

Further work: Tests

Checklist:

Here is a checklist that should completed before merging this given feature.
Any shortcomings from the items below should be explained and detailed within the contents of this PR.

  • I am proud of this feature.
  • I have performed a self-review of my own code.
  • I have manually tested this for different scenarios (different models, formats, devices, browsers).
  • I have commented my code in hard-to-understand areas.
  • I have made corresponding changes to the public documentation.
  • I have added unit and visuals tests to prove that my feature works to the best of my ability.
  • I have refactored the code for readability to the best of my ability.
  • I have checked that my changes do not introduce regressions in the public documentation.
  • I have outlined any known defects / lacking capabilities in the contents of this PR.
  • I have listed any remaining work that should follow this PR in the description or jira/miro/etc.
  • I have added TSDoc to any public facing changes.
  • I have added "developer documentation" in any package README.md that is related / applicable to this PR.
  • I have noted down and am currently tracking any technical debt introduced in this PR.
  • I have thoroughly thought about the architecture of this implementation.
  • I have asked peers to test this feature.

@haakonflatval-cognite haakonflatval-cognite requested a review from a team as a code owner July 7, 2022 15:06
@haakonflatval-cognite haakonflatval-cognite added the slack PRs and issues with this label will be pushed to Slack label Jul 7, 2022
@lgtm-com
Copy link

lgtm-com bot commented Jul 7, 2022

This pull request introduces 1 alert when merging 4f4bc0b into 0b2c2aa - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jul 12, 2022

This pull request introduces 1 alert when merging e539df4 into 60a80e3 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #2270 (63118d1) into feat/point-cloud-styling (d765a15) will increase coverage by 0.20%.
The diff coverage is 52.52%.

@@                     Coverage Diff                      @@
##           feat/point-cloud-styling    #2270      +/-   ##
============================================================
+ Coverage                     70.37%   70.58%   +0.20%     
============================================================
  Files                           271      283      +12     
  Lines                         11072    11387     +315     
  Branches                       1405     1475      +70     
============================================================
+ Hits                           7792     8037     +245     
- Misses                         3123     3177      +54     
- Partials                        157      173      +16     
Impacted Files Coverage Δ
viewer/.eslintrc.js 0.00% <ø> (ø)
viewer/index.ts 0.00% <0.00%> (ø)
viewer/internals.ts 0.00% <0.00%> (ø)
viewer/jest.config.js 0.00% <ø> (ø)
viewer/packages/api/src/migration.ts 0.00% <ø> (ø)
viewer/packages/api/src/public/RevealManager.ts 73.45% <ø> (ø)
...kages/api/src/public/migration/RenderController.ts 100.00% <ø> (ø)
viewer/packages/api/src/public/types.ts 100.00% <ø> (ø)
viewer/packages/api/src/utilities/Spinner.ts 84.37% <ø> (ø)
...ewer/packages/api/src/utilities/ViewStateHelper.ts 59.32% <ø> (ø)
... and 129 more

@haakonflatval-cognite haakonflatval-cognite force-pushed the hflatval/libraries-in-workers branch from 60a80e3 to a3cfbb3 Compare July 18, 2022 07:14
@haakonflatval-cognite haakonflatval-cognite force-pushed the hflatval/object-bounding-volume-hierarchy branch from fee66b3 to a14ccf5 Compare July 18, 2022 08:06
@github-actions
Copy link

There were failures in the examples workflow. This usually means a visual regression test has failed. Image diffs for visual tests can be downloaded as an artifact here. If there are no artifacts there's an error somewhere else in the examples workflow. If you have made intentional changes you can update the image snapshots by running yarn snapshots:update in examples/

@larsmoa
Copy link
Contributor

larsmoa commented Jul 19, 2022

Change description of PR to describe what we achieve (it's currently how ;) )

@lgtm-com
Copy link

lgtm-com bot commented Jul 27, 2022

This pull request introduces 1 alert when merging d5ccdc0 into a3cfbb3 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@haakonflatval-cognite haakonflatval-cognite changed the title feat: object bounding volume hierarchy feat: speed up point-object assignment with a bounding volume hierarchy Aug 1, 2022
Base automatically changed from hflatval/libraries-in-workers to feat/point-cloud-styling August 2, 2022 10:49
@haakonflatval-cognite haakonflatval-cognite force-pushed the hflatval/object-bounding-volume-hierarchy branch from b9b6a81 to 16a279d Compare August 8, 2022 08:08
@lgtm-com
Copy link

lgtm-com bot commented Aug 8, 2022

This pull request introduces 5 alerts when merging 16a279d into 1ce4078 - view on LGTM.com

new alerts:

  • 5 for Unused variable, import, function or class

@haakonflatval-cognite
Copy link
Contributor Author

We have arrived at the conclusion that we want this merged into the feature branch now. It is not as good as we would want, but it provides an enormous improvement over the previous approach.

That being said, there is quite a bit of garbage lying around. The BVH implementation is unused and the octree is tested. A lot of superfluous out-commented lines are still in assignObjects.ts for remembrance. The cleanup work is being tracked in https://cognitedata.atlassian.net/browse/REV-374

@haakonflatval-cognite
Copy link
Contributor Author

Just to make it clear here, this PR is intended to be superseded by #2327 , and will be closed as soon that one is merged.

@haakonflatval-cognite
Copy link
Contributor Author

Closing, as it is superseded by #2327

@haakonflatval-cognite haakonflatval-cognite deleted the hflatval/object-bounding-volume-hierarchy branch October 11, 2022 06:38
@haakonflatval-cognite haakonflatval-cognite restored the hflatval/object-bounding-volume-hierarchy branch October 11, 2022 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
slack PRs and issues with this label will be pushed to Slack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants