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

[sld-viewer] Simplify computations for navigation arrow #34

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

flo-dup
Copy link
Contributor

@flo-dup flo-dup commented Feb 23, 2024

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)

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

What kind of change does this PR introduce?
Refactor / simplification

What is the current behavior?

  • the arrow ordinate is computed by filling two id-ordinate maps in a loop over all navigable nodes (nodes with a non-null nextVId): one map for computing the max ordinate for each node id, one map for computing the min ordinate for each node id
  • the unique list of vls displayed is computed by looping over all the nodes
  • the computation of the list of unique vls is done with a complex indexof (yet classic!) which is not commented

What is the new behavior (if this is a feature change)?

  • y is directly read like x already was. Indeed, the same element id shouldn't be twice in the metadata, and even if it appears twice by mistake the querySelector would return the same svg element, hence the same ordinate.
  • the unique list of vls displayed is computed by looping over the navigable nodes: if one navigable node points to a vl displayed there is a navigable node in this vl (on the other side)
  • the computation of the list of unique vls is done with a Set

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

  • Yes
  • No

@flo-dup flo-dup changed the title Simplify ordinate computation for navigation arrow [sld-viewer] Simplify computations for navigation arrow Feb 23, 2024
@flo-dup flo-dup requested a review from CBiasuzzi May 16, 2024 13:56
Copy link
Contributor

@CBiasuzzi CBiasuzzi left a comment

Choose a reason for hiding this comment

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

With this change, the navigation arrows' vertical positions could be different, compared to the current logic's output.

The first loop should be more correctly interpreted as "one map for computing the max ordinate for each node's VL id, one map for computing the min ordinate for each node's VL id" (instead of "one map for computing the max ordinate for each node id, one map for computing the min ordinate for each node id" ).

The two maps contain the max y and min y values for all the VL involved, and these 2 values are the values that are used to place the navigation arrows in the diagram.
With the original logic, all the TOP arrows are aligned on the same top ordinate (likewise, all the BOTTOM arrows will be aligned on the same bottom ordinate)
With the proposed code, the arrows' ordinate values could vary from navigable node to navigable node.

In this example I changed in the svg file the y value of one of the 'navigable' nodes.
Output with the current implementation

image

Output with the proposed logic

image

Note that it depends on how the svg was generated: I edited the y value of a node in the svg file by hand, to create this scenario;
With the original svg, the arrows were all aligned at the same ordinate also using the proposed logic.

@flo-dup
Copy link
Contributor Author

flo-dup commented May 21, 2024

Indeed! I missed the fact that the map was indexed by the voltage level id...

The SVG constructed by powsybl-single-line-diagram has always y position equals for all the feeder nodes within the same voltage level: it corresponds to the top extern cell height or the bottom extern cell heigh which are both constant within a voltage level. So we could keep this simplification anyway

@CBiasuzzi
Copy link
Contributor

It's not always the case, apparently. In the SVG created with this code, the two bottom elements have different y:

Network network = Importer.find("CGMES")
        .importData(CgmesConformity1Catalog.microGridBaseCaseBE().dataSource(), new NetworkFactoryImpl(), null);
//let's create an SLD for the 4th VL
SingleLineDiagram.draw(network, "b10b171b-3bc5-4849-bb1f-61ed9ea1ec7c", "/tmp/sld-example.svg");

with the new logic for the diagram-viewer, the bottom arrows are not aligned:

sld1_new_logic

Copy link

sonarcloud bot commented Sep 18, 2024

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