-
Notifications
You must be signed in to change notification settings - Fork 17
Improve rendering performance by switching SVG rendering of O/D matrix to Canvas #584
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
Improve rendering performance by switching SVG rendering of O/D matrix to Canvas #584
Conversation
|
It would be great if the user manual were updated when this improvement is implemented. I could help by recording some nice demonstration videos. |
src/app/services/analytics/origin-destination/components/origin-destination.component.ts
Outdated
Show resolved
Hide resolved
src/app/services/analytics/origin-destination/components/origin-destination.component.ts
Show resolved
Hide resolved
src/app/services/analytics/origin-destination/components/origin-destination.component.ts
Outdated
Show resolved
Hide resolved
"that looks like a commit message rather than documentation" -> indeed it was just for my own
Make more clear why we check for existance
`That's confusing to me, since filtering trains should trigger computation` - comment was not aligned with the functionality (copy & paste issue)
Some benchmarks (all tests have finished : without error)51 Nodes - standalone demo
204 nodes - 3x duplicated - 4x standalone demo
408 Nodes
816 Nodes
Big, real Netzgrafik testI tested with an internal really big Netzgrafik (851 unique named nodes) - can not share the file/data! main branch (bdcc0d2)
aeg/od_improve_rendering_performance_switching_svg_to_canvasAfter very long waiting - time manuel measured (clock): 5min 38s = 338s)
just mouse over : real-time (near) High memory usage : 911MB Further idea to improve: Next steps=> Thus the reloadData cost are massiv ! -> might we can reduce it with open draft pull request (Dijkstra, ... ) |
src/app/view/editor-tools-view-component/editor-tools-view.component.ts
Outdated
Show resolved
Hide resolved
src/app/view/editor-tools-view-component/editor-tools-view.component.ts
Outdated
Show resolved
Hide resolved
src/app/view/editor-tools-view-component/editor-tools-view.component.ts
Outdated
Show resolved
Hide resolved
src/app/view/editor-tools-view-component/editor-tools-view.component.ts
Outdated
Show resolved
Hide resolved
src/app/view/editor-tools-view-component/editor-tools-view.component.ts
Outdated
Show resolved
Hide resolved
louisgreiner
left a comment
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.
A few comments, could not get into the PNG/SVG export
Also did not challenged the new structure of the component, but except for the ".style" noise, it's quite easy to follow actually
Note: origin-destination.component.html -> move the style outside of this file and set it in the .scss file
| style=" | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 1rem; | ||
| width: 100%; | ||
| height: 57px; | ||
| flex-wrap: nowrap; | ||
| " |
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.
Not only related to this styling, but can we keep style in the .scss corresponding files? Otherwise, it makes the code way more difficult to navigate into
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.
style is still html hard coded, can it be moved to scss file?
src/app/services/analytics/origin-destination/components/origin-destination.component.ts
Outdated
Show resolved
Hide resolved
| .attr("class", "tooltip") | ||
| .style("opacity", 0) | ||
| .style("position", "absolute") | ||
| .style("border", "solid 1px") | ||
| .style("border-radius", "4px") | ||
| .style("padding", "6px") | ||
| .style("user-select", "none") | ||
| .style("pointer-events", "none"); |
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.
Can we add this style as we do for other Netzgrafik-Editor objects? what is done with static.dom.tags.ts for instance; to reduce the code's length
-> same for other .style occurrences of this file
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.
:-)
src/app/services/analytics/origin-destination/components/origin-destination.component.ts
Show resolved
Hide resolved
src/app/services/analytics/origin-destination/components/origin-destination.component.html
Outdated
Show resolved
Hide resolved
src/app/services/analytics/origin-destination/components/origin-destination.component.ts
Outdated
Show resolved
Hide resolved
src/app/services/analytics/origin-destination/components/origin-destination.component.ts
Outdated
Show resolved
Hide resolved
src/app/services/analytics/origin-destination/components/origin-destination.component.ts
Outdated
Show resolved
Hide resolved
src/app/services/analytics/origin-destination/components/origin-destination.component.ts
Outdated
Show resolved
Hide resolved
src/app/services/analytics/origin-destination/components/origin-destination.component.ts
Outdated
Show resolved
Hide resolved
louisgreiner
left a comment
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.
A few more minor comments
| <div id="main-origin-destination-container-root"></div> | ||
|
|
||
| <div class="palette-buttons"> | ||
| <div class="palette-buttons" style="transform: translateX(-64px)"> |
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.
Same as other style parameters, can we move them to scss file?
| <div class="palette-buttons" style="transform: translateX(-64px)"> | |
| <div class="palette-buttons"> |
| style=" | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 1rem; | ||
| width: 100%; | ||
| height: 57px; | ||
| flex-wrap: nowrap; | ||
| " |
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.
style is still html hard coded, can it be moved to scss file?
| <sbb-radio-group | ||
| class="sbb-checkbox-group-horizontal" | ||
| [(ngModel)]="displayBy" | ||
| style="display: flex; flex-direction: row; gap: 0.5rem; transform: translateX(-16px)" |
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.
same issue, style must go to scss file
| style="display: flex; flex-direction: row; gap: 0.5rem; transform: translateX(-16px)" |
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.
done
| // ------------------------- | ||
| // Initialization helpers | ||
| // ------------------------- | ||
|
|
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.
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.
done
| return this.getStreckengrafikEditingContainerToExport(); | ||
| case EditorMode.OriginDestination: | ||
| return this.getOriginDestinationContainerToExport(); | ||
| return undefined; |
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.
Did not test this part, but this gives me bad feeling; could we gather all the exports at the same place?
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.
The problem is, that we have a complete different export behavior when using canvas ... but i will refactor it, try to fix the problem
|
Also, if doable I think it is better to let the reviewer press "Resolve conversation", to ensure the comment has been treated and there is no more discussion. Also, if the conversation are closed, I cannot see them on the "Files changed" page to review another time |
louisgreiner
left a comment
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.
I'm fine with the current code (except the few remarks), however the zoom feature is a bit bugged. You can try zoom on the edges of this network graphic for instance:
reticulaire(2).json
Also, when zoomed out a lot (30%), the panning feature is very slow, very different of the previous behaviour
Enregistrement.de.l.ecran.2025-10-30.163511.mp4
| // canvas Data-URL (PNG) | ||
| const dataUrl = canvas.toDataURL("image/png"); | ||
|
|
||
| // Erzeuge SVG string mit eingebettetem Bild |
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.
Can you translate this in english please 😆
| "text-anchor", | ||
| "dominant-baseline", | ||
| ]; | ||
| // quality only used for image/jpeg |
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.
What is the point of jpeg since we use png?
|
The current version is really fast. But pan/zoom doesn't work smooth and acceptable |
|
I experimented extensively with this pull request (PR), but unfortunately, I couldn't cleanly implement the transformation/scaling logic for panning and zooming. I also have to admit that while Canvas has its strengths, SVG offers significantly higher rendering quality—and the entire mesh graphics editor is based on SVG. Therefore, using Canvas for the o/d matrix is problematic. Performance optimizations for large matrices are possible, but would likely require techniques like frustum culling (as used in the mesh graphics editor's editor mode) and possibly other rendering strategies like level-of-detail (LOD). Furthermore, Canvas introduces considerable complexity once we consider HiDPI and Retina displays. ConclusionIn conclusion, this was a valuable experiment, and I learned a lot. While Canvas can significantly improve performance, it also introduces greater complexity and doesn't fit well with the SVG-based architecture of the mesh graphics editor. Decision (suggestion)Therefore, I propose closing this PR without merging it. Should Canvas become relevant again in the future, we can revisit this code and the discussion as a reference point. Thanks toMany thanks to everyone involved, including the reviewers! |














Description
Rendering of the origin/destination matrix was a major performance bottleneck. To address this, the SVG-based rendering of the O/D matrix has been replaced with a Canvas-based implementation.
To discuss - but it looks very promising
Please compare the rendering outcome: Pixel graphics (new) vs Vector graphics (old). If we like to have high quality support (vector graphic / svg) we might have to implement an automatic switch from (old) to new if the matrix gets to big.
I strongly recommend using canvas -> just using this canvas based rendering for such matrix based rendering otherwise we will run into big issue with the performance. I am aware that it's not so cool having pixel graphics but is looks quite good. And as well it make againt the interactive tool more complex du of having svg ( d3.js) and canvas in one product. But for interactive real - time application the life is hard:)
What changed
Known issues / TODOs
Export to PNG/SVG currently does not work. Export functionality needs to be restored for the new Canvas renderer.
Issue fixed - png can directly exported - svg must be simulated with an image layer but it works.
Small visual issue: some "button" elements render without a background. It appears the rendering area for those elements is too large, causing their backgrounds not to be drawn correctly.
Checklist
documentation/Fixed known issue
Issue with ambiguous node names fixed:
Final result
chrome-capture-2025-10-22.webm