-
Notifications
You must be signed in to change notification settings - Fork 6
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
Edges and Overlays Optimizations #141
Conversation
… code if logical groups array has length 0
I think that the overlay LOD management needs to be refactored out to some node overlay superclass. |
src/renderer/renderer_elements.ts
Outdated
// These two fields get set in the layouter, depending on the number of in/out_connectors | ||
// of a node. They also get toggled in the mousehandler when the hover status changes. | ||
// Currently only used for NestedSDFGs and ScopeNodes. | ||
public summarise_in_edges: boolean = false; |
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.
In the dace codebase, we use American English for spelling.
@@ -528,7 +530,7 @@ function file_read_complete(sdfv: SDFV): void { | |||
sdfv.get_renderer()?.destroy(); | |||
sdfv.set_renderer(new SDFGRenderer(sdfv, sdfg, container, mouse_event)); | |||
sdfv.close_menu(); | |||
}, 20); | |||
}, 10); |
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.
why this change?
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.
This should have been 10 ms from the beginning like all the other setTimeout() I added. Does not make a difference as 10 ms is enough for the browser to schedule the function for later.
src/renderer/renderer.ts
Outdated
} | ||
); | ||
this.emit('collapse_state_changed', true, true); | ||
this.relayout(); |
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.
Why relayout twice?
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.
Oh yes this was a quick fix because there was an error without the this.relayout() in-between the fully collapse and expand. I just took the existing collapse and expand code from the renderer. Relayouting after fully collapsing here only takes a millisecond, so if it is really necessary it's only a small overhead. I will check again though!
src/renderer/renderer.ts
Outdated
this.emit('collapse_state_changed', true, true); | ||
this.relayout(); | ||
|
||
// Expand by one level |
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 not sure about this mode for simple SDFGs, where we will start with states expanded but contents of maps collapsed. This might be good for huge graphs but some kernels will just require users to double click nodes, which some users do not know they can. @phschaad Thoughts?
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 could expand graphs depending on how large they are or add "expand" symbols to collapsed nodes. The main reasons for starting fully collapsed are the loading time when opening a file (54 seconds for the 255 MiB graph on my laptop) and readability of deeply nested graphs.
Yes, Philipp and I already discussed this. At the moment with my thesis I focus on the performance optimizations, and I checked the Overlay system in the profiler and only found little overhead with the current setup. Depending on the time I have left after the main part of my thesis, refactoring the Overlays could be an additional task I could work on. |
Branch is NOT READY FOR MERGE YET as there is still a runtime error. Also I still need to look at the startup collapse & expand. |
Need to first merge april fixes 2 to fix the runtime error, then this pull request is ready to merge into master. Collapsed startup is commented out for reworking it in a later commit. |
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.
LGTM in general, only two small corrections. Also it would be really nice if the PR description had an image demoing the edge summary :-)
this.updateCFGList(); | ||
|
||
// Create the initial SDFG layout |
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.
Since we're leaving this out for now, please remove this commented out section for this PR
src/renderer/renderer.ts
Outdated
@@ -3043,7 +3119,7 @@ export class SDFGRenderer extends EventEmitter { | |||
highlighting_changed = true; | |||
hover_changed = true; | |||
} | |||
|
|||
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.
fmt
Before
Sorted
Summarized