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

feat: show multiple diagram instances on a grid in editor #1287

Merged
merged 29 commits into from Feb 9, 2023
Merged

feat: show multiple diagram instances on a grid in editor #1287

merged 29 commits into from Feb 9, 2023

Conversation

wodeni
Copy link
Member

@wodeni wodeni commented Feb 2, 2023

Description

This PR introduces a new Grid panel to editor, which show multiple diagram instances at once. The number of instances is 10 by default, and configuration through the option panel.

synthesizer-ui first introduced a grid of Penrose-generated diagrams in a web app. This PR pulls relevant components (i.e. Gridbox and Grid) into components and uses them in both editor and synthesizer-ui.

Implementation strategy and design decisions

  • Grid contains Gridbox, which contains Simple. The properties are passed through, with Gridbox inheriting all Simple's props and Grid having a gridboxProps option for each Gridbox.
  • Gridbox can be stateful, which means that it caches the trio it receives from props and caches state from Simple's onFrame callback.
  • Styling: use styled-components to style Grid and Gridbox. The client may provide a theme using <ThemeProvider>.
  • editor's state management: the IDE wraps GridPanel around Grid and maintains a diagramGridState. All state updates are done through Recoil.
  • Image and Line arrowhead id collisions: since SVG has a global namespace, when there are multiple diagrams on the page with duplicated defs, they disappear. This PR introduces an util function makeIDsUnique, which changes all definitions and corresponding references of IRI references in the SVG such that they are unique. (Resolves Name collisions in SVG Image import #1288)

Examples with steps to reproduce them

See preview deployment:
grid

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new ESLint warnings
  • I have reviewed any generated registry diagram changes

Open questions

  • Caching in editor: the grid doesn't cache anything other than a list of variations, which causes re-rendering whenever the grid size is decreased and then increased. No re-rendering happen when the gridSize is decreasing because there are no prop changes.

@wodeni wodeni marked this pull request as draft February 2, 2023 05:42
@github-actions
Copy link

github-actions bot commented Feb 2, 2023

± Registry diff

M	3d-projection-fake-3d-linear-algebra.svg
M	arrowheads-arrowheads.svg
M	caffeine-structural-formula.svg
M	collinear-euclidean.svg
M	continuousmap-continuousmap.svg
M	parallel-lines-euclidean.svg
M	persistent-homology-persistent-homology.svg
M	siggraph-teaser-euclidean-teaser.svg
M	tree-tree.svg
M	tree-venn-3d.svg
M	two-vectors-perp-vectors-dashed.svg
M	vector-wedge-exterior-algebra.svg
M	wos-laplace-estimator-walk-on-spheres.svg
M	wos-nested-estimator-walk-on-spheres.svg
M	wos-offcenter-estimator-walk-on-spheres.svg
M	wos-poisson-estimator-walk-on-spheres.svg

📊 Performance

Key

Note that each bar component rounds up to the nearest 100ms, so each full bar is an overestimate by up to 400ms.

     0s   1s   2s   3s   4s   5s   6s   7s   8s   9s
     |    |    |    |    |    |    |    |    |    |
name ▝▀▀▀▀▀▀▀▀▀▀▀▚▄▄▄▄▄▄▄▄▄▞▀▀▀▀▀▀▀▀▀▀▀▀▚▄▄▄▄▄▄▄▄▄▖
      compilation labelling optimization rendering

Data

                                                   0s   1s   2s   3s   4s   5s   6s   7s   8s
                                                   |    |    |    |    |    |    |    |    |
3d-projection-fake-3d-linear-algebra               ▝▞▖
allShapes-allShapes                                ▝▚▚▄▄▄
arrowheads-arrowheads                              ▝▞▖
caffeine-structural-formula                        ▝▀▀▚▀▄
center-shrink-circle-animation                     ▝▞▖
circle-example-euclidean                           ▝▀▚▀▀▀▖
collinear-euclidean                                ▝▀▞▖
congruent-triangles-euclidean                      ▝▀▀▚▚
continuousmap-continuousmap                        ▝▚▚
cubic-bezier-cubic-bezier                          ▝▚▀▀▚
glutamine-molecules-basic                          ▝▀▚▚
half-adder-distinctive-shape                       ▝▚▀▖
hypergraph-hypergraph                              ▝▀▀▀▀▞▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▖
incenter-triangle-euclidean                        ▝▀▚▚
lagrange-bases-lagrange-bases                      ▝▚▚
midsegment-triangles-euclidean                     ▝▀▞▖
mobius-mobius                                      ▝▞▖
non-convex-non-convex                              ▝▀▞▖
one-water-molecule-atoms-and-bonds                 ▝▞▖
parallel-lines-euclidean                           ▝▀▞▚
persistent-homology-persistent-homology            ▝▀▀▚▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▚▖
points-around-line-shape-distance                  ▝▀▀▞▖
points-around-polyline-shape-distance              ▝▀▀▞▖
points-around-star-shape-distance                  ▝▀▀▀▚▚
quaternion-group-group-theory-cayley-graph         ▝▀▚▚
quaternion-group-group-theory-multiplication-table ▝▀▀▀▄▞▄
siggraph-teaser-euclidean-teaser                   ▝▀▚▚
small-graph-disjoint-rect-line-horiz               ▝▀▚▚
small-graph-disjoint-rects                         ▝▞▖
small-graph-disjoint-rects-large-canvas            ▝▚▚
small-graph-disjoint-rects-small-canvas            ▝▞▖
tree-tree                                          ▝▀▚▚
tree-venn                                          ▝▀▚▚
tree-venn-3d                                       ▝▀▞▄▄
two-triangles-triangle-mesh-3d                     ▝▚▚
two-vectors-perp-vectors-dashed                    ▝▚▚
vector-wedge-exterior-algebra                      ▝▚▚
wet-floor-atoms-and-bonds                          ▝▚▀▀▖
word-cloud-example-word-cloud                      ▝▀▞▖
wos-laplace-estimator-walk-on-spheres              ▝▀▚▚
wos-nested-estimator-walk-on-spheres               ▝▀▀▀▞▀▀▀▖
wos-offcenter-estimator-walk-on-spheres            ▝▀▚▀▀▀▀▚
wos-poisson-estimator-walk-on-spheres              ▝▀▚▀▀▖

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #1287 (3d163ec) into main (1895d8f) will decrease coverage by 0.46%.
The diff coverage is 4.41%.

@@            Coverage Diff             @@
##             main    #1287      +/-   ##
==========================================
- Coverage   63.04%   62.59%   -0.46%     
==========================================
  Files          61       62       +1     
  Lines        7301     7357      +56     
  Branches     1715     1732      +17     
==========================================
+ Hits         4603     4605       +2     
- Misses       2614     2661      +47     
- Partials       84       91       +7     
Impacted Files Coverage Δ
packages/core/src/renderer/Image.ts 5.55% <0.00%> (+1.70%) ⬆️
packages/core/src/renderer/Renderer.ts 25.92% <ø> (ø)
packages/core/src/renderer/util.ts 3.17% <3.17%> (ø)
packages/core/src/renderer/Line.ts 2.83% <25.00%> (-0.03%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cloudflare-pages
Copy link

cloudflare-pages bot commented Feb 2, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3d163ec
Status: ✅  Deploy successful!
Preview URL: https://9fd517fb.penrose-72l.pages.dev
Branch Preview URL: https://grid.penrose-72l.pages.dev

View logs

@wodeni wodeni marked this pull request as ready for review February 3, 2023 21:52
@samestep
Copy link
Collaborator

samestep commented Feb 6, 2023

Looks like CI is failing: https://github.com/penrose/penrose/actions/runs/4108063039/jobs/7088302283

TypeError: uuid.v4 is not a function

Any idea why?

@wodeni
Copy link
Member Author

wodeni commented Feb 7, 2023

Turns out that bumping uuid to 9.0 everywhere fixed the CI. My guess is a lower version is hoisted somehow.

Copy link
Collaborator

@samestep samestep left a comment

Choose a reason for hiding this comment

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

Still reviewing the code itself, but had a quick comment while playing with it: would it be possible to disable the "replace diagram with variation when you click on it" in editor? Or if that would be too much implementation burden for this PR, could we make a GitHub issue to do that after this PR is merged?

@samestep
Copy link
Collaborator

samestep commented Feb 8, 2023

Is the code for packages/core/src/renderer/util.ts copied from somewhere? If so, could you add a comment to the top of that file with a link to where we got it from, and also the copyright notice and license if applicable?

@samestep
Copy link
Collaborator

samestep commented Feb 8, 2023

Why does packages/examples/src/walk-on-spheres/walk-on-spheres-ball.svg change?

@samestep
Copy link
Collaborator

samestep commented Feb 8, 2023

I'm getting this error in Storybook for Continuous Map under Grid Component:

TypeError: this.props.header is not a function
    at http://localhost:6006/src/Grid.tsx:58:26
    at Array.map (<anonymous>)
    at Grid.innerContent (http://localhost:6006/src/Grid.tsx:50:32)
    at Grid.render (http://localhost:6006/src/Grid.tsx:89:21)
    at finishClassComponent (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:14267:39)
    at updateClassComponent (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:14232:32)
    at beginWork (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:15463:22)
    at beginWork$1 (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:19248:22)
    at performUnitOfWork (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:18693:20)
    at workLoopSync (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:18632:13)

@samestep
Copy link
Collaborator

samestep commented Feb 8, 2023

Similarly I get this error in Storybook under GridBox Component, not right away, but as soon as I click on the diagram:

TypeError: Cannot read properties of undefined (reading 'map')
    at Gridbox.render (http://localhost:6006/src/Gridbox.tsx:178:41)
    at finishClassComponent (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:14267:39)
    at updateClassComponent (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:14232:32)
    at beginWork (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:15463:22)
    at beginWork$1 (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:19248:22)
    at performUnitOfWork (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:18693:20)
    at workLoopSync (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:18632:13)
    at renderRootSync (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:18611:15)
    at recoverFromConcurrentError (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:18233:28)
    at performSyncWorkOnRoot (http://localhost:6006/node_modules/.vite-storybook/deps/chunk-4QL7BUP5.js?v=6233ef11:18375:28)

Copy link
Collaborator

@samestep samestep left a comment

Choose a reason for hiding this comment

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

Looks great! Left a few comments here and above. I'm cool to merge this once the questions about packages/core/src/renderer/util.ts and Storybook are resolved.

packages/components/src/Simple.tsx Show resolved Hide resolved
Copy link
Collaborator

@samestep samestep left a comment

Choose a reason for hiding this comment

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

Awesome, thanks Nimo!

packages/core/src/renderer/util.ts Outdated Show resolved Hide resolved
Co-authored-by: Sam Estep <sam@samestep.com>
@wodeni wodeni merged commit fbaf03c into main Feb 9, 2023
@wodeni wodeni deleted the grid branch February 9, 2023 13:23
This was referenced Feb 9, 2023
@wodeni wodeni mentioned this pull request Jun 7, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Name collisions in SVG Image import
2 participants