8000 feat: simplify webgl line series to use SDF by chrisprice · Pull Request #1718 · d3fc/d3fc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: simplify webgl line series to use SDF #1718

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
Merged
merged 2 commits into from
May 30, 2022
Merged

feat: simplify webgl line series to use SDF #1718

merged 2 commits into from
May 30, 2022

Conversation

chrisprice
Copy link
Contributor

The original implementation was prone to introducing artefacts when adjacent line segments had extreme changes in gradient (as noted in #1716). These can be seen in the snapshot diff below (left is the original, right is the SDF version) -

index-js-should-match-the-image-snapshot-1-diff

This is a reimplementation of the line series using SDFs. It improves on the existing performance by approximately 20% (most likely due to not rendering the additional artefacts).

Full regression test results 
npm run examples -- --testPathPattern series-webgl-line 

> examples
> cd examples && npx jest --runInBand "--testPathPattern" "series-webgl-line"

 FAIL  series-webgl-line/__tests__/index.js
  ✕ should match the image snapshot (647ms)
  ✕ should have consistent performance (3186ms)

  ● should match the image snapshot

    Expected image to match or be a close match to snapshot but was 3.386666666666667% different from snapshot (16256 differing pixels).
    See diff for details: /Users/chris/dev/d3fc/examples/series-webgl-line/__tests__/__image_snapshots__/__diff_output__/index-js-should-match-the-image-snapshot-1-diff.png

      4 |         omitBackground: true
      5 |     });
    > 6 |     expect(image).toMatchImageSnapshot();
        |                   ^
      7 |     await d3fc.saveScreenshot(module, image);
      8 | });
      9 | 

      at Object.<anonymous> (series-webgl-line/__tests__/index.js:6:19)

  ● should have consistent performance

    Expected 39.23333330949148 +/- 6.961666665474574 for "draw-duration-0" but got 31.233333349227905

      28 |     }
      29 |     Object.entries(averages).forEach(([name, { sum, count }]) => {
    > 30 |         expect(sum / count).toMatchPerformanceSnapshot(name);
         |                             ^
      31 |     });
      32 |     return { pass: true, message: () => '' };
      33 | };

      at forEach (__helpers__/toHaveConsistentPerformance.js:30:29)
          at Array.forEach (<anonymous>)
      at Object.<anonymous>.exports.toHaveConsistentPerformance (__helpers__/toHaveConsistentPerformance.js:29:30)
      at Object.<anonymous> (series-webgl-line/__tests__/index.js:11:5)

 › 2 snapshots failed.
Snapshot Summary
 › 2 snapshots failed from 1 test suite. Inspect your code changes or run `npm run npx -- -u` to update them.

Test Suites: 1 failed, 1 total
Tests:       2 failed, 2 total
Snapshots:   2 failed, 7 passed, 9 total
Time:        4.744s, estimated 5s

@chrisprice chrisprice force-pushed the sdf-line branch 3 times, most recently from 5901644 to 9a4caa4 Compare October 21, 2021 10:29
@sleighsoft
Copy link

Would love to see this go through? Any reason this has been stuck in PR mode so long?

ColinEberhardt
ColinEberhardt approved these changes May 30, 2022
View reviewed changes
Copy link
Member
@ColinEberhardt ColinEberhardt left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@chrisprice chrisprice merged commit f6212a3 into master May 30, 2022
@chrisprice chrisprice deleted the sdf-line branch May 30, 2022 19:38
@github-actions github-actions bot mentioned this pull request Jul 11, 2024
Merged
This was referenced Jul 26, 2024
Closed
Merged
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ColinEberhardt ColinEberhardt ColinEberhardt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chrisprice @sleighsoft @ColinEberhardt @cprice-scottlogic
0