Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: improve performance of all node styling functions in Cognite3DModel #1186

Merged
merged 6 commits into from
Oct 8, 2020

Conversation

larsmoa
Copy link
Contributor

@larsmoa larsmoa commented Oct 8, 2020

This improves performance when the user repeatedly calls Cognite3dModel.setNodeColorByTreeIndex and other node style modification functions by batching calls to requestNodeUpdate.

A good test case is the following (which can be used in the documentation):

const start = performance.now();
const promises = [];
for (let i = 0; i < 1000000; i++) {
  promises.push(model.setNodeColorByTreeIndex(i, 255, 255, 255));
}
Promise.all(promises).then(() => {
  window.alert('Took ' + (performance.now() - start) + ' ms');
});

In my test the time is reduced from 4.5 s to about 1 s.

@larsmoa larsmoa requested a review from a team as a code owner October 8, 2020 10:28
@larsmoa larsmoa added auto-merge auto-update Makes bulldozer automatically update this PR when there are changes to the target branch labels Oct 8, 2020
@github-actions
Copy link

github-actions bot commented Oct 8, 2020

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

await treeIndices.forEach(idx => this.selectedNodes.add(idx));
this.cadNode.requestNodeUpdate(treeIndices);
return treeIndices.count;
if (applyToChildren) {
Copy link
Contributor

Choose a reason for hiding this comment

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

See above comment

@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #1186 into master will decrease coverage by 0.51%.
The diff coverage is 25.18%.

@@            Coverage Diff             @@
##           master    #1186      +/-   ##
==========================================
- Coverage   64.25%   63.74%   -0.52%     
==========================================
  Files          94       95       +1     
  Lines        4247     4330      +83     
  Branches      416      414       -2     
==========================================
+ Hits         2729     2760      +31     
- Misses       1515     1567      +52     
  Partials        3        3              
Impacted Files Coverage Δ
viewer/src/public/migration/Cognite3DModel.ts 16.01% <1.94%> (-2.93%) ⬇️
viewer/src/public/migration/NodeStyleUpdater.ts 100.00% <100.00%> (ø)
viewer/src/utilities/NumericRange.ts 100.00% <100.00%> (ø)

@github-actions
Copy link

github-actions bot commented Oct 8, 2020

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/

@cognite-bulldozer cognite-bulldozer bot merged commit a3a4331 into master Oct 8, 2020
@cognite-bulldozer cognite-bulldozer bot deleted the larsmoa/faster-node-style-setters branch October 8, 2020 13:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
auto-update Makes bulldozer automatically update this PR when there are changes to the target branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants