Skip to content

[nad-viewer] Use SVG writer for creating nodes and edges in adaptive zoom functionality#423

Draft
massimo-ferraro wants to merge 5 commits into
integration/nad_create_svgfrom
nad_create_svg_adaptive_zoom
Draft

[nad-viewer] Use SVG writer for creating nodes and edges in adaptive zoom functionality#423
massimo-ferraro wants to merge 5 commits into
integration/nad_create_svgfrom
nad_create_svg_adaptive_zoom

Conversation

@massimo-ferraro

Copy link
Copy Markdown
Contributor

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • A PR or issue has been opened in all impacted repositories (if any)

Does this PR already have an issue describing the problem?
no

What kind of change does this PR introduce?
feature

What is the current behavior?
The adaptive zoom functionality handles text boxes and edge infos, removing and redrawing them according to thresholds. It doesn't handle nodes and edges.

What is the new behavior (if this is a feature change)?
The adaptive zoom functionality handles also nodes and edges, removing and redrawing them according to thresholds. The thresholds could also be associated to voltage levels, for removing and redrawing nodes and edges according to their voltage level

Does this PR introduce a breaking change or deprecate an API?

  • Yes
  • No

If yes, please check if the following requirements are fulfilled

  • The Breaking Change or Deprecated label has been added
  • The migration steps are described in the following section

What changes might users need to make in their application due to this PR? (migration steps)
The adaptive zoom functionality has been changed for handling nodes and edges. New sections of the AdaptiveTextZoomOptions has been added for configuring the handling of nodes and edges, an example of configuration can be found in realgrid.ts file in the demo.
A new parameter for choosing the creation of the SVG from the diagram metadata has also been added to the NAD parameters

…zoom functionality

Signed-off-by: massimo.ferraro <massimo.ferraro@soft.it>
Signed-off-by: massimo.ferraro <massimo.ferraro@soft.it>
@flo-dup

flo-dup commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

It looks very promising, great work @massimo-ferraro ! 👏

Some quick comments on this draft, you are probably aware already but here it comes:

  • the svgContent should now be optional in the viewer constructor as it's not needed if metadata is here - I think it should be the default way to use the component. We could keep a viewer with a svg without metadata as input, but I doubt it's of any use
  • the nad-edge-infos are below the nad-branch-edges in the svg, leading to arrows being below edges
image
  • the text edges should be optional, we should probably add a parameter in SvgParameters in powsybl-diagram for them

  • I had the feeling that sometimes the visible viewbox is wrong, or at least the visible elements: I saw some nodes missing at some zoom levels. I can't reproduce it though, did you see something like that? maybe not due to viewbox?

  • Should we add a bit of margin when computing the viewbox? for the dragging part, but in fact it's so quick to generate it when releasing the mouse click that it might not be worth it

  • The viewer seems to still be dependent on the viewbox of the original svg

    • when I give an empty svg with no viewbox the wheel-zooming is not really working
    • when I give an empty svg with only the viewbox (and no width/height) the dragging shows that the width taken to write the svg is a bigger than what is visible
  • about the demo generating java part, I can't launch the generation of the realGrid data, there seems to be a problem when targeting the zip resource with ResourceDataSource. Besides we should avoid adding a binary file to github

But all that seems to be minor bugs, it's a very promising work! 👌 It was the goal of your long work on integration/nad_create_svg and of @CBiasuzzi work on adaptive zoom text and powsybl-diagram metadata, congratulations already for these first results to both of you - can't wait to see that work merged and integrated.

Comment on lines +2175 to +2179
const vlThreshold = DiagramUtils.getVLThreshold(adaptiveTextZoom.nodeThresholds, maxDisplayedSize);
const previousVlThreshold = DiagramUtils.getVLThreshold(
adaptiveTextZoom.edgeThresholds,
this.getPreviousMaxDisplayedSize()
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why is vlThreshold calculated with nodeThresholds but previousVlThreshold is calculated with edgeThreshold ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, I should have used the edgeThresholds for the edges filtering, not the nodeThresholds (the previousVlThreshold is needed only for the edge filtering). Fixed,

};
}

export function getMaxThreshold(voltageLevelThreshols: VoltageLevelThreshold[]): VoltageLevelThreshold {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
export function getMaxThreshold(voltageLevelThreshols: VoltageLevelThreshold[]): VoltageLevelThreshold {
export function getMaxThreshold(voltageLevelThresholds: VoltageLevelThreshold[]): VoltageLevelThreshold {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed the typo

);
}

export function getMinThreshold(voltageLevelThreshols: VoltageLevelThreshold[]): VoltageLevelThreshold {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
export function getMinThreshold(voltageLevelThreshols: VoltageLevelThreshold[]): VoltageLevelThreshold {
export function getMinThreshold(voltageLevelThresholds: VoltageLevelThreshold[]): VoltageLevelThreshold {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed the typo

}

export function getVLThreshold(
voltageLevelThreshols: VoltageLevelThreshold[],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
voltageLevelThreshols: VoltageLevelThreshold[],
voltageLevelThresholds: VoltageLevelThreshold[],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed the typo

Comment on lines +422 to +425
return (voltageLevelThreshols.length == 0 ? [{ threshold: 500000 }] : voltageLevelThreshols).reduce(
(a, b) => (a.threshold > b.threshold ? a : b),
voltageLevelThreshols[0] ?? [{ threshold: 500000 }]
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

how about something like that:

if (length === 0) {
   return {threshold: 500000}
} else {
   return voltageLevelThresholds.reduce(...)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed for using the if else, even if in the short-hand version, but anyway it avoids to reduce an array with only one element

}

export function getMaxThreshold(voltageLevelThreshols: VoltageLevelThreshold[]): VoltageLevelThreshold {
return (voltageLevelThreshols.length == 0 ? [{ threshold: 500000 }] : voltageLevelThreshols).reduce(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the 500000 is already defined in nad-viewer-parameters:

static readonly THRESHOLD_NODES_ADAPTIVE_ZOOM_DEFAULT = 500000;
static readonly THRESHOLD_EDGES_ADAPTIVE_ZOOM_DEFAULT = 500000;

Why not use this static value ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now I use the static value

Comment on lines 177 to 182
this.nadViewerParameters = new NadViewerParameters(nadViewerParametersOptions ?? undefined);
if (this.nadViewerParameters.getCreateSvgFromMetadata() && this.diagramMetadata != null) {
this.svgWriter = new SvgWriter({ diagramMetadata: this.diagramMetadata });
this.svgContent = this.svgWriter.getEmptySvg();
}
this.nadViewerParameters = new NadViewerParameters(nadViewerParametersOptions ?? undefined);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
this.nadViewerParameters = new NadViewerParameters(nadViewerParametersOptions ?? undefined);
if (this.nadViewerParameters.getCreateSvgFromMetadata() && this.diagramMetadata != null) {
this.svgWriter = new SvgWriter({ diagramMetadata: this.diagramMetadata });
this.svgContent = this.svgWriter.getEmptySvg();
}
this.nadViewerParameters = new NadViewerParameters(nadViewerParametersOptions ?? undefined);
this.nadViewerParameters = new NadViewerParameters(nadViewerParametersOptions ?? undefined);
if (this.nadViewerParameters.getCreateSvgFromMetadata() && this.diagramMetadata != null) {
this.svgWriter = new SvgWriter({ diagramMetadata: this.diagramMetadata });
this.svgContent = this.svgWriter.getEmptySvg();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the second new NadViewerParameters overwrites the first one, I don't think this is intended, correct me if I'm wrong

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, removed the duplicated line

}
}

filterNodes(nodes: NodeMetadata[], vlThreshold: VoltageLevelThreshold, maxDisplayedSize: number): NodeMetadata[] {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why is this not private ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Likely created by the IDE, I now made it private

return nodes;
}

filterEdges(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why is this not private ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Likely created by the IDE, I now made it private

Comment on lines +448 to +450
export function getVLThresholdClasses(voltageLevels: string[]): string {
return '.' + voltageLevels.join(',.');
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't see this used anywhere, do you plan to use it for something later ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, I created and used this method during my developments. In the end I removed the usage, but forgot to remove the function. Removed now

Signed-off-by: massimo.ferraro <massimo.ferraro@soft.it>
@massimo-ferraro

Copy link
Copy Markdown
Contributor Author

It looks very promising, great work @massimo-ferraro ! 👏

Thanks

Some quick comments on this draft, you are probably aware already but here it comes:

* the svgContent should now be optional in the viewer constructor as it's not needed if metadata is here - I think it should be the default way to use the component. We could keep a viewer with a svg without metadata as input, but I doubt it's of any use

Now, in this branch, we have:

  • the svg writer, that can create the whole svg, starting from the metadata, and ignoring the svg content provided as input. This operation is done according to nad parameter createSvgFromMetadata. This, creating everything in the svg, takes some time to create the whole real grid
  • the adaptive zoom functionality, that uses the svg writer to create part of the svg (nodes and edges). This functionality, at least in my tests, starts from the svg content provided as input, removes what needs to be removed and redraws what needs to be redrawn

If you would like to start without the content, and create what is needed in the init, adapting the first situation (the svg writer that creates the whole svg) to work with filtered elements and some voltage levels would require some time and some work.
Another option, quicker to implement, would be to use the adaptive zoom funtionality for this goal: at the init, if the adaptive zoom is enebaled (or should we add a new nad parameter?) you do not use the input content, create an empty svg (but with the correct viewbox and size, as you noted they are needed), and let the adaptive zoom functionality to fill the svg with the necessary elements (likely as you already did in your tests).
Should we proceed this way?

* the nad-edge-infos are below the nad-branch-edges in the svg, leading to arrows being below edges
image

Fixed

* the text edges should be optional, we should probably add a parameter in SvgParameters in powsybl-diagram for them

If this parameter is added, it could be used by the adaptive zoom, and also for the whole SVG creation from metadata

* I had the feeling that sometimes the visible viewbox is wrong, or at least the visible elements: I saw some nodes missing at some zoom levels. I can't reproduce it though, did you see something like that? maybe not due to viewbox?

I saw something like that with some combination of maxWidth input parameter and browser zoom. I tried to change how the maxWidth input parameter is computed (see realgrid.ts), but it could still have happened if you changed the browser zoom after loading the page. I tried changing the css class of the window in the real grid html page, it could have solved the problem (to be tested). btw. this computation of the width could also be put inside the viewer, instead of in the code that calls the creation of the viewer

* Should we add a bit of margin when computing the viewbox? for the dragging part, but in fact it's so quick to generate it when releasing the mouse click that it might not be worth it

* The viewer seems to still be dependent on the viewbox of the original svg
  
  * when I give an empty svg with no viewbox the wheel-zooming is not really working
  * when I give an empty svg with only the viewbox (and no width/height) the dragging shows that the width taken to write the svg is a bigger than what is visible

* about the demo generating java part, I can't launch the generation of the realGrid data, there seems to be a problem when targeting the zip resource with ResourceDataSource. Besides we should avoid adding a binary file to github

But all that seems to be minor bugs, it's a very promising work! 👌 It was the goal of your long work on integration/nad_create_svg and of @CBiasuzzi work on adaptive zoom text and powsybl-diagram metadata, congratulations already for these first results to both of you - can't wait to see that work merged and integrated.

Signed-off-by: massimo.ferraro <massimo.ferraro@soft.it>
Signed-off-by: massimo.ferraro <massimo.ferraro@soft.it>
@sonarqubecloud

Copy link
Copy Markdown

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.

3 participants