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

improvement: avoid stalling rendering by adding asynchronous picking #2191

Merged
merged 60 commits into from
Jun 22, 2022

Conversation

larsmoa
Copy link
Contributor

@larsmoa larsmoa commented Jun 7, 2022

Description

Avoid blocking main thread when doing intersection tests to reduce "staggering" when doing picking. Also fixed a typo in PR template.

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.
    Unit test not added. Spent some time trying, but could not create a sensible test that actually tested anything substantial.
  • 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.
  • 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 when I have been in doubt.
    Tested Chrome, Safari, Chrome Canary, iOS Safari.

@larsmoa larsmoa added auto-update Makes bulldozer automatically update this PR when there are changes to the target branch slack PRs and issues with this label will be pushed to Slack labels Jun 7, 2022
@github-actions
Copy link

github-actions bot commented Jun 7, 2022

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
Copy link

github-actions bot commented Jun 7, 2022

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
Copy link

github-actions bot commented Jun 7, 2022

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/

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #2191 (8b6dd39) into master (747aa2a) will decrease coverage by 0.28%.
The diff coverage is 21.87%.

@@            Coverage Diff             @@
##           master    #2191      +/-   ##
==========================================
- Coverage   71.01%   70.73%   -0.29%     
==========================================
  Files         254      255       +1     
  Lines       10826    10879      +53     
  Branches     1407     1415       +8     
==========================================
+ Hits         7688     7695       +7     
- Misses       2979     3021      +42     
- Partials      159      163       +4     
Impacted Files Coverage Δ
...ckages/api/src/public/migration/Cognite3DViewer.ts 59.27% <0.00%> (ø)
...cad-model/src/picking/readPixelsFromTargetAsync.ts 9.80% <9.80%> (ø)
...r/packages/cad-model/src/picking/PickingHandler.ts 65.82% <75.00%> (+0.88%) ⬆️

@larsmoa larsmoa added the preview-docs Deploy preview documentation for a PR label Jun 9, 2022
@github-actions
Copy link

github-actions bot commented Jun 9, 2022

📙 Documentation preview is available from
https://cognitedata.github.io/reveal-docs-preview/2191/docs/next/.

@larsmoa larsmoa enabled auto-merge (squash) June 22, 2022 11:00
@larsmoa
Copy link
Contributor Author

larsmoa commented Jun 22, 2022

Looks good, but can't say I see much change in performance on this branch vs. master. Do you have a tip for a good test scenario to make the difference shine through more?

Not really. It won't feel very different in most cases as it only helps when GPU has a lot of work to do. This helps in those situations by yielding control back to main thread so e.g. UI can be updated before picking results are ready.

Copy link
Contributor

@haakonflatval-cognite haakonflatval-cognite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@larsmoa larsmoa merged commit a4d35b8 into master Jun 22, 2022
@larsmoa larsmoa deleted the larsmoa/async-pick branch June 22, 2022 14:40
haakonflatval-cognite added a commit that referenced this pull request Jul 4, 2022
* chore: bump reveal version 3.x doc (#2231)

* chore: bump reveal version 3.x doc

* Updated 3.x docs

Co-authored-by: Savokr <savelii.novikov@cognite.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency @cognite/sdk to v7.6.2 (#2220)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency @cognite/sdk to v7.7.0 (#2232)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency three to v0.141.0 (#2162)

* fix(deps): update dependency three to v0.141.0

* fix: undo documentation three bump

* chore: bump three for examples

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Christopher Tannum <christopher.tannum@cognite.com>

* refactor: port core package to api package (#2192)

* chore: move file type mocks to test utilities

* refactor: remove unused types

* refactor: move all internal export to root

* refactor: port core to new api package

* fix: restore build

* fix: ignore typing error from geomap

* fix: restore tests

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix(deps): update dependency three to v0.141.0 (#2234)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* improvement: avoid stalling rendering by adding asynchronous picking (#2191)

* fix: backport threejs for documentation to fix v1 doc (#2237)

* chore(deps): update dependency gl to v5.0.3 (#2238)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* test: added test for Cognite3dViewer.dispose() (#2141)

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* improvement: hide/show HTML overlay elements (#2240)

* Added API to show/hide HTMLOverlay elements

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency lint-staged to v13.0.3 (#2241)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix: mobile default settings should not include ssao, add iPad fix (#2239)

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix: in-front objects incorrectly blending with background (#2248)

* fix: in-front objects now incorrectly blending

* chore: update visual tests

* chore: bump to Webpack 5 (#2244)

* feat: migrate examples to webpack-5

* improvement: simplify and de-deprecate config-overrides

* feat: upgrade to webpack 5 in viewer

* fix: correct watchOptions.ignored field

* fix: visual tests in examples

* feat: update webpack config for example apps

* chore: add temporary entrypoint+dependency in webpack conf

* fix: make examples correctly pick up three js

* fix: replace react-rewired stuff with own wepback config

* improvement: add log of which visual test is running

* fix: use better OpenGL backend in puppeteer

* test: update visual test images

* fix: fix test images after updating GPU backend

* improvement: simplify viewer webpack config

* chore: lint fix

* fix: add environment variable for production build

* feat: also serve content from examples/public

* fix: try backporting pupeteer webgl backend for fixing CI tests

* chore: attempt to build reveal in prod for documentation

* chore: remove unused variable and update label

* chore: update documentation yarn.lock

* fix: restore tests from wrong merge

* fix: update visual tests after merge... again...

Co-authored-by: Håkon Flatval <hakon.flatval@cognite.com>

* chore(deps): update dependency ts-loader to v9.3.1 (#2249)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency typedoc-plugin-markdown to v3.13.1 (#2247)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency core-js to v3.23.3 (#2243)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update typescript-eslint monorepo to v5.30.0 (#2246)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency eslint-plugin-prettier to v4.1.0 (#2245)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix: improper linking of viewer into examples (#2252)

* fix: elements position in HTMLOverlay when visibility is changed from hide to show (#2251)

* Fixed elements position, after unhiding the elements from hidden state

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency @types/node to v16.11.42 (#2253)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency eslint-plugin-prettier to v4.2.1 (#2255)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency webpack-dev-server to v4.9.3 (#2254)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix: incorrect node transform override on certain boundary conditions (#2256)

* refactor: convert transform override texture to floating point data texture

* refactor: convert transform override index pointer texture to floating point data texture

* fix: reallocating override texture adding wrong available matrix indices

* chore: add visual test snap

* fix: change obsolete threejs getInverse to repair example

* chore: remove unused float packing shader utility

* refactor: remove magic number

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix: sector repository gracefully handle sectors loader errors (#2257)

* chore(deps): update dependency nock to v13.2.8 (#2258)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency typedoc-plugin-markdown to v3.13.2 (#2259)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

Co-authored-by: Christopher J. Tannum <christopher.tannum@cognite.com>
Co-authored-by: Savokr <savelii.novikov@cognite.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Lars Moastuen <lars.moastuen@cognite.com>
Co-authored-by: Pramod S <87521752+pramodcog@users.noreply.github.com>
haakonflatval-cognite added a commit that referenced this pull request Jul 15, 2022
* chore: bump reveal version 3.x doc (#2231)

* chore: bump reveal version 3.x doc

* Updated 3.x docs

Co-authored-by: Savokr <savelii.novikov@cognite.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency @cognite/sdk to v7.6.2 (#2220)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency @cognite/sdk to v7.7.0 (#2232)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency three to v0.141.0 (#2162)

* fix(deps): update dependency three to v0.141.0

* fix: undo documentation three bump

* chore: bump three for examples

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Christopher Tannum <christopher.tannum@cognite.com>

* refactor: port core package to api package (#2192)

* chore: move file type mocks to test utilities

* refactor: remove unused types

* refactor: move all internal export to root

* refactor: port core to new api package

* fix: restore build

* fix: ignore typing error from geomap

* fix: restore tests

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix(deps): update dependency three to v0.141.0 (#2234)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* improvement: avoid stalling rendering by adding asynchronous picking (#2191)

* fix: backport threejs for documentation to fix v1 doc (#2237)

* chore(deps): update dependency gl to v5.0.3 (#2238)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* test: added test for Cognite3dViewer.dispose() (#2141)

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* improvement: hide/show HTML overlay elements (#2240)

* Added API to show/hide HTMLOverlay elements

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency lint-staged to v13.0.3 (#2241)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix: mobile default settings should not include ssao, add iPad fix (#2239)

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix: in-front objects incorrectly blending with background (#2248)

* fix: in-front objects now incorrectly blending

* chore: update visual tests

* chore: bump to Webpack 5 (#2244)

* feat: migrate examples to webpack-5

* improvement: simplify and de-deprecate config-overrides

* feat: upgrade to webpack 5 in viewer

* fix: correct watchOptions.ignored field

* fix: visual tests in examples

* feat: update webpack config for example apps

* chore: add temporary entrypoint+dependency in webpack conf

* fix: make examples correctly pick up three js

* fix: replace react-rewired stuff with own wepback config

* improvement: add log of which visual test is running

* fix: use better OpenGL backend in puppeteer

* test: update visual test images

* fix: fix test images after updating GPU backend

* improvement: simplify viewer webpack config

* chore: lint fix

* fix: add environment variable for production build

* feat: also serve content from examples/public

* fix: try backporting pupeteer webgl backend for fixing CI tests

* chore: attempt to build reveal in prod for documentation

* chore: remove unused variable and update label

* chore: update documentation yarn.lock

* fix: restore tests from wrong merge

* fix: update visual tests after merge... again...

Co-authored-by: Håkon Flatval <hakon.flatval@cognite.com>

* chore(deps): update dependency ts-loader to v9.3.1 (#2249)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency typedoc-plugin-markdown to v3.13.1 (#2247)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency core-js to v3.23.3 (#2243)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update typescript-eslint monorepo to v5.30.0 (#2246)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency eslint-plugin-prettier to v4.1.0 (#2245)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix: improper linking of viewer into examples (#2252)

* fix: elements position in HTMLOverlay when visibility is changed from hide to show (#2251)

* Fixed elements position, after unhiding the elements from hidden state

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency @types/node to v16.11.42 (#2253)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency eslint-plugin-prettier to v4.2.1 (#2255)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency webpack-dev-server to v4.9.3 (#2254)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix: incorrect node transform override on certain boundary conditions (#2256)

* refactor: convert transform override texture to floating point data texture

* refactor: convert transform override index pointer texture to floating point data texture

* fix: reallocating override texture adding wrong available matrix indices

* chore: add visual test snap

* fix: change obsolete threejs getInverse to repair example

* chore: remove unused float packing shader utility

* refactor: remove magic number

Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix: sector repository gracefully handle sectors loader errors (#2257)

* chore(deps): update dependency nock to v13.2.8 (#2258)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* chore(deps): update dependency typedoc-plugin-markdown to v3.13.2 (#2259)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>

* fix: manually loop unroll in shader to improve performance (#2265)

* improvement: better error message when adding model fails (#2266)

* chore(deps): update dependency jest to v28 (#2088)

* chore(deps): update dependency jest to v28

* chore: fix broken github actions for preview docs comment on Github (#2276)

* chore: fix broken github actions for preview docs comment on Github

* Use GitHub CLI

* Replace action task with Github CLI

* Checkout to provide con

* Use correct Ruby

* Go for something that works

* Use of forked comment-on-pr

* Use our own fork with release tag

* lint

Co-authored-by: Savelii Novikov <kvaka2000@gmail.com>

Co-authored-by: Christopher J. Tannum <christopher.tannum@cognite.com>
Co-authored-by: Savokr <savelii.novikov@cognite.com>
Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Lars Moastuen <lars.moastuen@cognite.com>
Co-authored-by: Pramod S <87521752+pramodcog@users.noreply.github.com>
Co-authored-by: Savelii Novikov <kvaka2000@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-update Makes bulldozer automatically update this PR when there are changes to the target branch preview-docs Deploy preview documentation for a PR 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.

2 participants