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

Hide edges on viewport animation #2477

Closed
ghost opened this issue Jul 19, 2019 · 7 comments
Closed

Hide edges on viewport animation #2477

ghost opened this issue Jul 19, 2019 · 7 comments
Milestone

Comments

@ghost
Copy link

ghost commented Jul 19, 2019

Issue type

Feature request

Description of new feature

Setting hideEdgesOnViewport to true in the initialization options can give a performance improvement in very large graphs when the user pans and zooms. However, when the viewport is animated with cy.animate(), the edges remain visible.

Animating the viewport of very large graphs may be more performant if edges are hidden during the animation.

Motivation for new feature

I have a large graph (~1k nodes and ~6k edges). Visualizing it is quite performant, but animating it isn't at all.

It's possible to do a workaround: you can, before animating, update the style to set edges to visibility: hidden and then back to visibility: visible in cy.animate()'s callback. However, performance here is unpredictable -- some animations just jump, others are quite smooth.

I'm not familiar with this project's codebase, but if someone can confirm that this should give a perf boost in theory, then the change should be as simple as changing this line to check if the graph is animated:

var vpManip = (r.pinching || r.hoverData.dragging || r.swipePanning || r.data.wheelZooming || r.hoverData.draggingEles);
var hideEdges = r.hideEdgesOnViewport && vpManip;
@maxkfranz
Copy link
Member

I think that the scope of the proposed feature should be expanded: An option to hide edges on any animation is not sufficient. Maybe it's sufficient for your use-case, but it could cause conflicts in others. For example, a consumer may want to animate one particular edge and hide the other edges.

(1) This feature should not be coupled with hideEdgesOnViewport .

(2) This feature should not be coupled to animation.

One approach would be to allow for manually setting when edges should be hidden. That's more flexible, but it has usability concerns: A consumer could mistakenly use something new showEdges() function errantly -- for example, expecting the function to show edges that have had display:none set.

Generally, I'd prefer to remove features like hideEdgesOnViewport from the library -- say for v4 -- and leave that sort of behaviour to extensions or app-level code.

@maxkfranz
Copy link
Member

Or do you mean that you want to hide edges for viewport-only animations, such as an animated pan?

@ghost
Copy link
Author

ghost commented Jul 25, 2019

Ah yes, sorry, I was not clear in the initial post. I just meant for animated pans/zooms.

@maxkfranz
Copy link
Member

maxkfranz commented Jul 30, 2019

Maybe we could just add cy.animated() as a condition within vpManip?

@ghost
Copy link
Author

ghost commented Jul 31, 2019

Yes, unless cy.animated() is also true when running e.g. an animated layout? In that case, hideEdgesOnViewport would hide the edges not only on viewport changes but on any animation.

@maxkfranz
Copy link
Member

maxkfranz commented Jul 31, 2019

cy.animated() is true if the viewport is animated (zoom/pan transform). It doesn’t matter what originated the animation. An animated layout may or may not animate the viewport. It depends whether you set the layout to manipulate the zoom/pan.

@maxkfranz maxkfranz changed the title Hide edges on animation Hide edges on viewport animation Aug 1, 2019
@maxkfranz maxkfranz added this to the 3.9.0 milestone Aug 1, 2019
maxkfranz added a commit that referenced this issue Aug 1, 2019
@maxkfranz
Copy link
Member

I tried the proposed change with cy.layout({ name: 'random', animate: true, fit: true }).run() on the debug page. It seems to work OK, so I'm including this for 3.9. I don't think any documentation changes are necessary.

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

No branches or pull requests

1 participant