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

Add pngFull function to Cytoscape.js Core with Visual Cues Enhancement #15

Merged
merged 12 commits into from
Jan 23, 2024

Conversation

LaraMerdol
Copy link
Contributor

Description:

This pull request proposes the addition of a new pngFull function enhancing the capabilities of exporting the graph to a PNG image by integrating visual cues.

Changes Made:

  • New Functionality: Added the pngFull function as a new Cytoscape.js core function.
  • Added export as PNG, PNG(hall) option to the demo

Notes:

  • Dependency Added: Included the html2canvas library as a new dependency to enable rendering of cues.
  • Dependency Note: Please note that the html2canvas library may not automatically recognize certain elements, such as 'cy-context-menus-cxt-menu' and 'cy-panzoom'.
  • Customization Requirement: To ensure proper functionality, it's necessary to manually add these elements to the ignoreElementClasses parameter within the pngFull method.
    pngFull(options: any, ignoreElementClasses: string[] = [])
  • Rendering time: When there are many cues in a graph, exploring the png may take longer.

Screenshots

Screen Shot 2024-01-14 at 22 20 30 Screen Shot 2024-01-14 at 22 21 25

Note

Copy link
Member

@canbax canbax left a comment

Choose a reason for hiding this comment

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

  1. There is a flood of console logs. Can you get rid of them?

#280 218ms Finished rendering
cytoscape-visual-cues.ts:869 #281 0ms Starting document clone with size 1728x915 scrolled to 0,0
html2canvas.esm.js:7624 #281 176ms Document cloned, element located at 317.1070556640625,740.471435546875 with size 77.43096923828125x78.6600341796875 using computed rendering
html2canvas.esm.js:7624 #281 176ms Starting DOM parsing

  1. This produces wrong images for "n18" node. See image below generated by this.
image

versus the image I see on the canvas

image
  1. I see this method is computationally very expensive and long and hard to detect errors. Why not call html2canvas on the cytoscape.js container element? This seems much faster and smaller. Below code works to get image of what you see. To get the full image of canvas, maybe we need to play with the function html2canvas( Can you try that?
export async function pngFull(isWholeScreen: boolean): Promise<string> {
  const cy = this;
  const cyZoom = cy.zoom();
  const container = cy.container();
  return (
    await html2canvas(container, {
      backgroundColor: "transparent",
      scale: isWholeScreen ? 1 / cyZoom : 1,
    })
  ).toDataURL();
}

demo/demo.html Outdated
@@ -103,6 +103,8 @@ <h2 class="accordion-header" id="panelsStayOpen-headingOne">
Save
</button>
<ul class="dropdown-menu" aria-labelledby="dropdownMenuButton1">
<li><a class="dropdown-item" href="#" onclick="exportPng()">PNG</a></li>
<li><a class="dropdown-item" href="#" onclick="exportPngAll()">PNG (hall)</a></li>
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: I guess "(hall)" is a typo, shouldn't it need to be "(all)"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the correction, it has been changed to "whole".
Screen Shot 2024-01-17 at 11 30 55

* @param {string[]} ignoreElementClasses - Array of classes to ignore.
* @returns combinedBase64 - Base64-encoded PNG of the graph.
*/
export async function pngFull(options: any, ignoreElementClasses: string[] = []) {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: define type or interface for options, don't use any. This will be more clear and type-strict

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion, the interface has been defined for PngOptions.

@canbax
Copy link
Member

canbax commented Jan 16, 2024

Also, another issue is the pipeline. Why it failed?

@LaraMerdol
Copy link
Contributor Author

    1. Console logs were removed, should I also remove the console errors?
    1. The issue is about the miscalculation of margins for the full option now it is fixed.
Screen Shot 2024-01-17 at 11 20 14
    1. The suggested implementation works well for the !full option. However, when rendering the entire graph, it seems necessary to render each cue separately and combine them with the image retrieved from png(full). The reason for this is that htmI2canvas only renders the visible part on the canvas, not the entire graph. Therefore, I have modified the implementation accordingly. Now, for the !full option, it will render the container as a whole, while for the full option, it will do the previous, more expensive implementation.

@LaraMerdol
Copy link
Contributor Author

Also, another issue is the pipeline. Why it failed?

Now the pipeline is running without any issues. I have added the "Install Dependencies" step before running the project. Is this okay?

@LaraMerdol LaraMerdol requested a review from canbax January 17, 2024 08:58
Copy link
Member

@canbax canbax left a comment

Choose a reason for hiding this comment

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

How about a different approach: If it's full first save the pan and zoom values of the canvas first. And then fit to canvas. And then wait some time so that the cues are positioned properly and then call html2canvas. After that revert the cytoscape pan and zoom values. The only disadvantage of this method is small flickering since we are changing pan and zoom values programmatically. But still, it's faster.

/**
 * 
 * @param wait milliseconds to wait
 * @returns 
 */
export function asyncWait(wait: number): Promise<boolean> {
  return new Promise((resolve) => {
    setTimeout(() => {
      resolve(true);
    }, wait);
  });
}

const prevPan = cy.pan();
 const prevZoom = cy.zoom();
 cy.fit();
 await asyncWait(150);
 const results = (
    await html2canvas(container, { logging: false })
 ).toDataURL();
 cy.zoom(prevZoom);
 cy.pan(prevPan);
 return results;

What do you think about this approach? Also please, call html2canvas with logging:false

@LaraMerdol
Copy link
Contributor Author

I have made the suggested changes. The approach works, but there is a slight flickering issue as you discussed earlier. In my opinion, I agree with you that this approach is less expensive and the flickering is not too bothersome.
Screen Recording 2024-01-21 at 21 44 27

Note: Also calling html2canvas with logging:false

@LaraMerdol LaraMerdol requested a review from canbax January 21, 2024 18:48
Copy link
Member

@canbax canbax left a comment

Choose a reason for hiding this comment

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

Nice work! It's great that also considers the cues margins in cy.fit() call.

Of course, there could be lots of improvements such as adding tests, creating separate files for types and utils, performance tests, code style etc... But I think those are different concerns.

@LaraMerdol LaraMerdol merged commit d1eb863 into main Jan 23, 2024
2 checks passed
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.

2 participants