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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from
Jun 30, 2022
Merged

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

merged 11 commits into from
Jun 30, 2022

Conversation

christjt
Copy link
Contributor

Description

Fixes a transform override issue at certain boundary conditions. Porting to floating point texture fixed it. So error was probably in pack/unpack float arithmetic.

For more context see: https://cognitedata.slack.com/archives/C01MZ6TPCH2/p1656439576826729

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.

@christjt christjt 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 preview-docs Deploy preview documentation for a PR labels Jun 30, 2022
@christjt christjt requested a review from a team as a code owner June 30, 2022 09:39
@codecov
Copy link

codecov bot commented Jun 30, 2022 •
edited
Loading

Codecov Report

Merging #2256 (40e249d) into master (077df3c) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

❗ Current head 40e249d differs from pull request most recent head 52264a6. Consider uploading reports for the commit 52264a6 to get more accurate results

@@            Coverage Diff             @@
##           master    #2256      +/-   ##
==========================================
- Coverage   71.58%   71.44%   -0.15%     
==========================================
  Files         255      254       -1     
  Lines       10897    10848      -49     
  Branches     1420     1414       -6     
==========================================
- Hits         7801     7750      -51     
- Misses       2937     2939       +2     
  Partials      159      159
Impacted Files Coverage Δ
viewer/packages/utilities/index.ts 100.00% <ø> (ø)
...ering/src/transform/NodeTransformTextureBuilder.ts 92.59% <100.00%> (-0.14%) ⬇️
...rendering/src/transform/TransformOverrideBuffer.ts 93.87% <100.00%> (-0.47%) ⬇️
.../packages/cad-parsers/src/utilities/SectorScene.ts 96.34% <0.00%> (-2.44%) ⬇️

@github-actions
Copy link

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

Savokr
Savokr approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@Savokr Savokr left a comment

Choose a reason for hiding this comment

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

Looks good, two minor comments but otherwise very nice change!

viewer/packages/rendering/src/transform/TransformOverrideBuffer.ts Outdated Show resolved Hide resolved
viewer/packages/utilities/src/packFloat.ts Show resolved Hide resolved
@christjt christjt enabled auto-merge (squash) June 30, 2022 14:13
@christjt christjt merged commit fe85758 into master Jun 30, 2022
@christjt christjt deleted the christjt/fix-transform-override branch June 30, 2022 14:22
haakonflatval-cognite added a commit that referenced this pull request Jul 4, 2022
@haakonflatval-cognite @christjt
@Savokr @cognite-bulldozer @renovate @larsmoa @pramodcog
chore: update point cloud feature branch (#2264)
5414b2a 
* 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
@haakonflatval-cognite @christjt
@Savokr @cognite-bulldozer @renovate @larsmoa @pramodcog
chore: update point cloud feature branch (#2277)
ac3e0f8 
* 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
Reviewers

@Savokr Savokr Savokr approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@christjt @Savokr