feat: Add ability to style objects in point clouds #2104

haakonflatval-cognite · 2022-04-29T13:13:13Z

No description provided.

lgtm-com · 2022-04-29T13:22:44Z

This pull request introduces 2 alerts when merging 6b6139f into dd3b620 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable
1 for Syntax error

lgtm-com · 2022-05-04T12:19:02Z

This pull request introduces 2 alerts when merging 94328b2 into 6fcc8cc - view on LGTM.com

new alerts:

1 for Useless assignment to local variable
1 for Unused variable, import, function or class

lgtm-com · 2022-05-05T13:42:32Z

This pull request introduces 1 alert when merging 50e0141 into ee5c380 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2022-05-05T14:11:34Z

This pull request introduces 1 alert when merging caf63a7 into ee5c380 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

lgtm-com · 2022-05-06T13:56:31Z

This pull request introduces 1 alert when merging bfa4c5f into ee5c380 - view on LGTM.com

new alerts:

1 for Expression has no effect

lgtm-com · 2022-05-18T15:12:49Z

This pull request introduces 2 alerts when merging 6c9f639 into 21c5693 - view on LGTM.com

new alerts:

1 for Expression has no effect
1 for Unused variable, import, function or class

lgtm-com · 2022-05-18T15:32:04Z

This pull request introduces 2 alerts when merging 358bef7 into 21c5693 - view on LGTM.com

new alerts:

1 for Expression has no effect
1 for Unused variable, import, function or class

haakonflatval-cognite · 2022-05-19T07:06:21Z

Features included at this point:

Simple concept of shapes and objects
Automatic assignment of point cloud points to their respective shapes
Simple styling (color) on a per-object basis
Automatic fetching of annotations ("stylable objects") from the annotations API on model load

codecov · 2022-05-19T07:18:56Z

Codecov Report

Merging #2104 (d765a15) into feat/point-cloud-styling (f59f8ac) will decrease coverage by 0.83%.
The diff coverage is 37.86%.

@@                     Coverage Diff                      @@
##           feat/point-cloud-styling    #2104      +/-   ##
============================================================
- Coverage                     71.20%   70.37%   -0.84%     
============================================================
  Files                           259      271      +12     
  Lines                         10794    11072     +278     
  Branches                       1364     1405      +41     
============================================================
+ Hits                           7686     7792     +106     
- Misses                         2955     3123     +168     
- Partials                        153      157       +4

Impacted Files	Coverage Δ
.../core/src/public/migration/renderOptionsHelpers.ts	`45.97% <ø> (ø)`
viewer/core/src/public/migration/types.ts	`100.00% <ø> (ø)`
viewer/index.ts	`0.00% <0.00%> (ø)`
viewer/packages/cad-model/src/CadManager.ts	`53.27% <0.00%> (-1.02%)`	⬇️
viewer/packages/cad-model/src/PickingHandler.ts	`64.93% <ø> (-0.45%)`	⬇️
...packages/modeldata-api/src/CdfModelDataProvider.ts	`80.76% <ø> (+5.76%)`	⬆️
...ckages/modeldata-api/src/LocalModelDataProvider.ts	`25.00% <ø> (+5.00%)`	⬆️
viewer/packages/modeldata-api/src/types.ts	`100.00% <ø> (ø)`
viewer/packages/pointclouds/index.ts	`100.00% <ø> (ø)`
...ewer/packages/pointclouds/src/PointCloudManager.ts	`48.64% <0.00%> (ø)`
... and 40 more

lgtm-com · 2022-05-19T13:27:09Z

This pull request introduces 1 alert when merging 5854f35 into 21c5693 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

github-actions · 2022-05-20T08:50:06Z

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/

github-actions · 2022-05-20T13:10:09Z

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/

github-actions · 2022-05-20T14:24:52Z

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/

christjt

Overall looks very good, and a great addition with high potential for impact. Not quite sure what to do with the playground SDK, and unless absolutely necessary, I don't think we can release it. As the barrier using CDF is already quite large as they need to authenticate themselves.

I still need to do some manual testing, but have some code comments you can look at in the mean time. Also probably a lot we can do a better job of unit testing here (ref. regression in code-coverage).

viewer/core/src/public/migration/Cognite3DViewer.ts

viewer/core/src/public/migration/types.ts

viewer/packages/pointclouds/src/PointCloudFactory.ts

viewer/packages/pointclouds/src/PointCloudManager.ts

viewer/packages/pointclouds/src/styling/annotationsToObjects.ts

viewer/packages/pointclouds/src/styling/shapes/IRawShape.ts

viewer/packages/pointclouds/src/styling/shapes/fromRawShape.ts

viewer/packages/pointclouds/src/styling/shapes/linalg.ts

viewer/packages/pointclouds/src/styling/shapes/IShape.ts

lgtm-com · 2022-05-24T11:38:23Z

This pull request fixes 2 alerts when merging b809e4b into 21c5693 - view on LGTM.com

fixed alerts:

2 for Unused variable, import, function or class

lgtm-com · 2022-05-24T14:36:11Z

This pull request introduces 1 alert and fixes 2 when merging 2b38883 into 21c5693 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

fixed alerts:

2 for Unused variable, import, function or class

lgtm-com · 2022-05-25T08:33:36Z

This pull request introduces 1 alert and fixes 2 when merging f5f8aeb into 21c5693 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

fixed alerts:

2 for Unused variable, import, function or class

lgtm-com · 2022-05-25T12:22:03Z

This pull request fixes 2 alerts when merging b15e42f into 21c5693 - view on LGTM.com

fixed alerts:

2 for Unused variable, import, function or class

lgtm-com · 2022-05-25T12:47:26Z

This pull request fixes 2 alerts when merging 9ce2801 into 21c5693 - view on LGTM.com

fixed alerts:

2 for Unused variable, import, function or class

lgtm-com · 2022-05-25T13:08:37Z

This pull request fixes 2 alerts when merging d8ad572 into 21c5693 - view on LGTM.com

fixed alerts:

2 for Unused variable, import, function or class

lgtm-com · 2022-05-25T14:10:14Z

This pull request fixes 2 alerts when merging 397ab09 into 21c5693 - view on LGTM.com

fixed alerts:

2 for Unused variable, import, function or class

lgtm-com · 2022-05-25T14:28:33Z

This pull request fixes 2 alerts when merging db398ca into 21c5693 - view on LGTM.com

fixed alerts:

2 for Unused variable, import, function or class

christjt

Sorry... Some of the comments are more suggestions than requested fixes.

christjt · 2022-05-25T14:42:30Z

viewer/packages/pointclouds/src/CognitePointCloudModel.test.ts

+import { createPointCloudModel } from '../../../test-utilities/src/createPointCloudModel';
+import { CognitePointCloudModel } from './CognitePointCloudModel';
+
+describe(CognitePointCloudModel.name, () => {


Feel like there is more that can be tested here 😅

viewer/packages/pointclouds/src/PointCloudManager.ts

viewer/packages/pointclouds/src/styling/annotationsToObjects.ts

viewer/packages/pointclouds/src/styling/shapes/Box.ts

viewer/packages/pointclouds/src/styling/shapes/CompositeShape.ts

viewer/packages/pointclouds/src/styling/shapes/Cylinder.ts

Co-authored-by: Renovate Bot <bot@renovateapp.com>

They'll be back for the acceleration structure

christjt

Accepting with a little doubt. Still feel the code quality of the point cloud library is a little brittle (I know you didn't write it), and we should probably take more ownership of it and refactor, simplify and remove anything we don't need. Also need to have a lot more testing going forward.

Other than that, I am really looking forward to seeing how this will look like in practice. Good job!

* chore(deps): update dependency @cognite/sdk to v7.4.0 (#2160) Co-authored-by: Renovate Bot <bot@renovateapp.com> * improvement: remove unused 'headers' for custom data sources and update docs (#2164) * fix: clearing wrong render target (#2166) * example: add UI for setting point size type (#2157) * improvement: Make typescript strict (#2163) * fix: fix all strict typechecking errors * improvement: don't return primitive collections if empty * chore: lint fix * improvement: make byteOffset 0 if undefined * improvement: unhackify helper function * fix: fix that byteOffset fix * fix: fix type errors for tests * fix: correct BinaryHeap bevaviour Co-authored-by: Christopher J. Tannum <christopher.tannum@cognite.com> Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com> * chore(deps): update dependency @cognite/sdk to v7.5.0 (#2165) Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com> * feat: simple styling in the works * feat: working visualization of pipe system Pipe data not included * feat: add simple styling interface for point clouds Also temporarily add data.ts to allow for testing for people who are not me * refac: make object style LUT 2D * refactor: let EPT loader take non-hardcoded object list * feat: automatically fetch annotations from server on point cloud load * chore: update Migration example for annotation fetching * fix: remove unused import * fix: remove not-yet-available feature * feat: add arbitrarily oriented boxes as stylable objects * refactor: some cleanup among stylable shapes * fix: don't import anything externally to worker * chore: lint fix * feat: blend style color with original RGB * chore: lint fix * chore: fix LGTM warnings * chore: update yarn lock * test: fix constructor in test * chore: remove unused member variable * chore: remove unused import Fixes LGTM error * fix: make @cognite/sdk-playground a peer dependency * fix: fix that pesky visual test error * refactor: factor out default EPT metadata file name * improvement: make "then" into an "await" * refactor: refactor annotation translation * chore: make public variable private-gettable * fix: correct import path * fix: refer to private field, not public getter * refactor: rewrite point cloud buffer parsing * improvement: be stricter in specifying optional types * improvement: make use of color constants * improvement: rewrite 0-vector check * fix: correct paratheses in shader * improvement: mappify array transformation * improvement: make ShapeType an enum * improvement: remove out-commented import * chore: lint fix * fix: make CogniteClientPlayground optionality annotation stricter * test: add trivial test for CognitePointCloudModel * fix: add missing creation function file * refactor: move some responsibility from PointCloudManager to -Factory * improvement: then -> await * fix: make indices be of type Uint32 * feat: add fromThreeVector3 utility function * refactor: parser-worker logic * improvement: make parse(data) actually return parsed data * improvement: improve readability * refactor: simplify primitive translation * improvement: renamed/simplify objectId assignment * refactor: change terminology from "StyledObject" to "StylableObject" * refactor: simplify Box constructor * chore: minor cleanup * improvement: simplify composite bounding box computation * fix: correct Box constructor call * fix: repair, please type checker, and lint after merge * improvement: remove computeBoundingBox for shapes for now They'll be back for the acceleration structure * chore: another lint fix * chore: add warning regarding point-object matching * refactor: remove "StylableObjectInfo" abstraction * chore: lint fix Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Lars Moastuen <lars.moastuen@cognite.com> Co-authored-by: Christopher J. Tannum <christopher.tannum@cognite.com> Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* feat: Add ability to style objects in point clouds (#2104) * feat: point cloud API for styling (#2179) * feat: Webassembly setup (#2353) * Update point cloud feature branch from master (#2416) * feat: Rust/Webassembly octree implementation for faster point-object assignment (#2327) * feat: custom classification (#2320) * docs: finalize point cloud styling docs (#2500) * refactor: move point cloud object provider to data-providers and expose object bounding boxes (#2522) * fix: point cloud custom transformation (#2550) * improvement: various fixes for point clouds (#2551) * fix: export point cloud object metadata (#2552) * Various big and small fixes Co-authored-by: Lars Moastuen <lars.moastuen@cognite.com> Co-authored-by: Christopher J. Tannum <christopher.tannum@cognite.com> Co-authored-by: Savokr <savelii.novikov@cognite.com> Co-authored-by: Pramod S <87521752+pramodcog@users.noreply.github.com>

haakonflatval-cognite force-pushed the hflatval/point-cloud-styling branch from 6b6139f to 94328b2 Compare May 4, 2022 12:14

haakonflatval-cognite force-pushed the hflatval/point-cloud-styling branch from 94328b2 to 50e0141 Compare May 5, 2022 13:37

haakonflatval-cognite force-pushed the hflatval/point-cloud-styling branch from 5f0b294 to 74be15f Compare May 18, 2022 15:35

haakonflatval-cognite changed the base branch from master to feat/point-cloud-styling May 19, 2022 07:03

haakonflatval-cognite marked this pull request as ready for review May 19, 2022 07:03

haakonflatval-cognite requested a review from a team as a code owner May 19, 2022 07:03

christjt requested changes May 23, 2022

View reviewed changes

christjt requested changes May 25, 2022

View reviewed changes

chore(deps): update dependency @cognite/sdk to v7.4.0 (#2160)

36c37dd

Co-authored-by: Renovate Bot <bot@renovateapp.com>

haakonflatval-cognite added 20 commits May 30, 2022 11:20

fix: make CogniteClientPlayground optionality annotation stricter

3dc9196

test: add trivial test for CognitePointCloudModel

8d5530d

fix: add missing creation function file

3d47eb6

refactor: move some responsibility from PointCloudManager to -Factory

eb12fd6

improvement: then -> await

323715a

fix: make indices be of type Uint32

d378bba

feat: add fromThreeVector3 utility function

e1bc4f9

refactor: parser-worker logic

dc4c192

improvement: make parse(data) actually return parsed data

4b40c6f

improvement: improve readability

602a4f7

refactor: simplify primitive translation

8b1c5e4

improvement: renamed/simplify objectId assignment

7052f95

refactor: change terminology from "StyledObject" to "StylableObject"

dbbecb5

refactor: simplify Box constructor

e05bdbf

chore: minor cleanup

2bbdeb8

improvement: simplify composite bounding box computation

614e136

fix: correct Box constructor call

da9f046

fix: repair, please type checker, and lint after merge

38c92b6

improvement: remove computeBoundingBox for shapes for now

72373e9

They'll be back for the acceleration structure

chore: another lint fix

a342d48

haakonflatval-cognite force-pushed the hflatval/point-cloud-styling branch from ba0b9cf to a342d48 Compare May 30, 2022 09:28

haakonflatval-cognite added 2 commits May 30, 2022 11:54

chore: add warning regarding point-object matching

3a25014

refactor: remove "StylableObjectInfo" abstraction

8e26eb7

christjt approved these changes May 30, 2022

View reviewed changes

chore: lint fix

d765a15

haakonflatval-cognite merged commit 02b9439 into feat/point-cloud-styling May 30, 2022

haakonflatval-cognite deleted the hflatval/point-cloud-styling branch May 30, 2022 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ability to style objects in point clouds #2104

feat: Add ability to style objects in point clouds #2104

haakonflatval-cognite commented Apr 29, 2022

lgtm-com bot commented Apr 29, 2022

lgtm-com bot commented May 4, 2022

lgtm-com bot commented May 5, 2022

lgtm-com bot commented May 5, 2022

lgtm-com bot commented May 6, 2022

lgtm-com bot commented May 18, 2022

lgtm-com bot commented May 18, 2022

haakonflatval-cognite commented May 19, 2022

codecov bot commented May 19, 2022 •

edited

Loading

lgtm-com bot commented May 19, 2022

github-actions bot commented May 20, 2022

github-actions bot commented May 20, 2022

github-actions bot commented May 20, 2022

christjt left a comment

lgtm-com bot commented May 24, 2022

lgtm-com bot commented May 24, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

christjt left a comment

christjt May 25, 2022

christjt left a comment

feat: Add ability to style objects in point clouds #2104

feat: Add ability to style objects in point clouds #2104

Conversation

haakonflatval-cognite commented Apr 29, 2022

lgtm-com bot commented Apr 29, 2022

lgtm-com bot commented May 4, 2022

lgtm-com bot commented May 5, 2022

lgtm-com bot commented May 5, 2022

lgtm-com bot commented May 6, 2022

lgtm-com bot commented May 18, 2022

lgtm-com bot commented May 18, 2022

haakonflatval-cognite commented May 19, 2022

codecov bot commented May 19, 2022 • edited Loading

Codecov Report

lgtm-com bot commented May 19, 2022

github-actions bot commented May 20, 2022

github-actions bot commented May 20, 2022

github-actions bot commented May 20, 2022

christjt left a comment

Choose a reason for hiding this comment

lgtm-com bot commented May 24, 2022

lgtm-com bot commented May 24, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

lgtm-com bot commented May 25, 2022

christjt left a comment

Choose a reason for hiding this comment

christjt May 25, 2022

Choose a reason for hiding this comment

christjt left a comment

Choose a reason for hiding this comment

codecov bot commented May 19, 2022 •

edited

Loading