-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix: remove flat modifier, while maintaining TreeIndex precision #2536
Merged
larsmoa
merged 8 commits into
cognitedata:master
from
equinor:feature/remove-flat-shading-while-keeping-precision
Oct 7, 2022
Merged
fix: remove flat modifier, while maintaining TreeIndex precision #2536
larsmoa
merged 8 commits into
cognitedata:master
from
equinor:feature/remove-flat-shading-while-keeping-precision
Oct 7, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 6053321.
Super hacky, but it avoids tree indexes around 2~3 million having accuracy issues. It adds quite a lot of operations, so maybe encoding a different way works better. This is a POC commit. The code should be cleaned up.
Strepto
changed the title
Remove flat modifier, while maintaining TreeIndex precision
fix: remove flat modifier, while maintaining TreeIndex precision
Oct 5, 2022
larsmoa
reviewed
Oct 5, 2022
Codecov Report
@@ Coverage Diff @@
## master #2536 +/- ##
==========================================
+ Coverage 70.60% 70.62% +0.02%
==========================================
Files 225 225
Lines 9753 9753
Branches 1271 1271
==========================================
+ Hits 6886 6888 +2
+ Misses 2713 2711 -2
Partials 154 154
|
Just to delay the refactor to int later
@larsmoa Does this look like what you wanted? |
larsmoa
approved these changes
Oct 6, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful @Strepto - thank you!
larsmoa
approved these changes
Oct 6, 2022
Will be automatically merge when you update your branch, @Strepto |
15 tasks
Strepto
deleted the
feature/remove-flat-shading-while-keeping-precision
branch
October 7, 2022 07:04
christjt
pushed a commit
that referenced
this pull request
Nov 2, 2022
Contribution by @Strepto (Equinor)
christjt
added a commit
that referenced
this pull request
Nov 3, 2022
…ch (#2635) * chore: add workflow that deploys to NPM on Github release (#2586) * test: initial dry run publish * chore: trigger release on released published * fix: remove caching on release workflow Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com> * fix: expose and fix bug causing getBoundingBoxByNodeId/TreeIndex not to modify out parameter (#2472) * refact: keyboard and mouse event handling in viewer (#2492) * Intermediate commit * Removed isFocused check in keyboard handling * Keyboard active only which domElement is focused, mouse events are active when canvas is focused * Refactored axisview eventlistener * Reverted axisview event passed to domElement * updated examples yarn.lock * clean up * leftover clean up * Keyboard event listen on domElement, removed unnecessary event push to domElement from Axis cross * removed unused variables * clean up * Fixed visual test error Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com> * fixed point cloud picking precision issue (#2508) Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com> * improvement: use 'high-performance' powerPreference to avoid non-discrete GPU being used on certain systems (#2512) * feat: add metrics to MeasurementTool (#2523) * chore: backport to old file structure * fix: remove flat modifier, while maintaining TreeIndex precision (#2536) Contribution by @Strepto (Equinor) * fix: do not blend in-front and back frame buffers when back objects have not been rendered (#2540) Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com> * fix: visualization artifacts due to wrongfully handling cylinder clip planes (#2556) * fix: simplify shader * fix: set proper plane length * fix: reduce some matrix transformations, and use built-in face-forward * fix: plane magnitude not adjusting for scaled world transform * fix: bad merge * chore: revert CDF environments json * chore: add primitive test fixture Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com> * improvement: make CadModelUpdateHandler trigger sector loading immediately after the camera stops moving (#2573) * Make CadModelUpdateHandler trigger sector loading immediately after the camera stops moving * Remove setting of redraw flag * Update test * Fix test Co-authored-by: Christopher J. Tannum <[email protected]> * Use vec2 instead of struct for packing TreeIndexes (#2585) This works around an issue with struct precision on ardeno gpus: KhronosGroup/WebGL#3351 * fix: general cylinder inside rendering (#2620) * fix: rendering if camera is inside primitive * fix: reverse normal direction if hitting inside of cylinder * fix: enable depth writing for in-front pass (#2621) * Enable depth writing for in-front pass * Set depth to minimum of in-front and back pass Co-authored-by: Christopher J. Tannum <[email protected]> * fix: remove unused pcMaterialManager from bad merge * fix: re-create v8 test image due to cylinder clipping planes fix Co-authored-by: cognite-bulldozer[bot] <51074376+cognite-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Lars Moastuen <[email protected]> Co-authored-by: Pramod S <[email protected]> Co-authored-by: Nils Henrik Hals <[email protected]> Co-authored-by: eiriklegernaes <[email protected]> Co-authored-by: Håkon Flatval <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This resolves an iOS Metal performance issue introduced by the fix of #2363 . The performance issue is caused by an implementation detail in the WebGL spec which is incompatible with Metal rendering, and WebKit has to emulates this detail by reshuffling vertices. Which is slow.
A real fix for this issue is in the works, with the EXT_PROVOKING_VERTEX extension, currently in a draft stage: https://registry.khronos.org/webgl/extensions/EXT_provoking_vertex/ . The extension is being implemented in WebKit (See WebKit/WebKit#3701 ), but I guess we cannot expect this to be shipped soon.
As the original problem here is that "varyings" in WebGL use an algorithm that averages the three sides of the vertex, and also applies a "Perspective Correction". See "For varyings though WebGL uses this formula" on https://webglfundamentals.org/webgl/lessons/webgl-3d-perspective-correct-texturemapping.html
Example showing the issue. (random example proving the above algorithm seems correct)
We get an offset result, but only for some values of W (and t). This varies, causing the "noise" pattern, seen in some screenshots of the "pre-fix" when tree-indices are high.
In this PR i propose a "dirty" fix, separating the TreeIndex into two different numbers in the Vertex Shader, and merging these numbers in the Fragment Shader. This removes the rounding issue given it works as the example above, as the inaccuracy does not occur for values below 2_000_000 in any of my tests.
The current maximum supported treeIndex is 2^24 due to storing it in an RGB255 Color, so we do not yet need to handle higher values than approx 16_800_000, which the proposed code will handle nicely given the formula above.
This PR adds another medp float to each fragment, and additional multiplications to add these together. The separation of 100ds and SubHundreds is kinda arbitrary, but medp has rounding issues when we get near 1000, so 99 as max works OK.
I might be overly defensive on rounding.
I have tested this with nodeIds up to 10_500_000, and have not spotted any issues with the approach.
If you have any suggestion on OTHER approaches to pass these bytes through the shader I would be happy, as this feels a bit wrong... :D
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.