Routingpoint information for newly created edges is not present on server side

[tortmayr]

See also: #730

If new edges created with the edge creation tool contain additional routing points, these routing points are only present on the client side but not on the server side (and therefore they are also not persisted into the source model).

Once the new edge is moved the server receives a ChangeRoutingPoints update is triggered and the server is updated with the correct routing information.

• Adapt GLSP Client
• Adapt GLSP Java Server
• Adapt GLSP Node Server

[martin-fleck-at]

After having a more detailed look into this, it seems this is not straight-forward to fix since the responsibilities between the server and client are shared during edge creation:

• The client sends the create edge action with the respective type as well as source and target.
• The server creates the necessary edge element and sets the router kind.
• The client can calculate the routing points based on the router kind.

Since there is no way for the client to know about the router kind before the edge has actually been created, we cannot send the routing points with the edge creation. Therefore we end up with a similar problem as we do with the computed bounds. However, in this case, I think it would be sufficient to provide a mechanism on the client that listens for the next model update and sends the routing point update after the edge creation. We could provide a custom action, e.g., PostUpdateAction that wraps another action and is then applied after the next model update through the action dispatcher. We may even be able to leverage the feedback action dispatcher for that, but this still needs to be investigated.

[planger]

That's an interesting problem! :-) I might not have the full picture, but maybe my initial thoughts help:

1. I'm wondering whether this really is a bug after all. There are no manual routing points after edge creation. The user currently can't specify them in the course of an edge creation, really. The routing points are dynamic routing points computed in a dynamic layouting step on the client. Leaving other tools than GLSP out of the picture for now, it may even be counter-useful to store these, as this information is not only redundant but also leads to unexpected behavior -- if a user would move the node later on and there are no manual routing points, the layout algorithm is free to choose a new route, whereas if there are manual routing points, it should actually respect those in some ways. So the server would have to be made aware of dynamic routing points and manual routing points, etc., which is a distinction we afaik currently don't have at all on the server.
2. To enable the server to also consider dynamic routing points in some way (e.g. to store them for exports compatible with other tools), another idea is to extend the ComputedBoundsAction and include the routing points information there. Not sure whether this should be done by default or left to adopters for customization, as this might be kind of a corner case?
3. If we would like to support the create edge action to specify routing points (e.g. with a custom tool where you can click on the canvas while creating the edge to set a routing point), I guess we could extend the create action with routing points. But then I'd tend to leave it as a responsibility of the adopter to make sure that those routing points are consistent (with the router kind, etc.).

Please consider these as rather uninformed thoughts as I haven't spent too much time on it. I'm just not quite sure if this is really a bug or rather a requirement that is quite specific to make it a default behavior or introduce another mechansim.

[rsoika]

I see it form a user perspective more like this:

• First I just want to draw a new edge between two nodes. Form the server side this is just a simple edge.
• The client provides the edge-routing-logic (mainly sprotty I think) and provides a suggestion of the best routing the client can find
• As a user I can accept it or a adjust it to my individual layout understanding

The default route computed form the GLSP client should be fine for the most cases and of course I want to store this valuable information in my model. (In fact, I plan to invest more time in optimizing the route for Manhattan routing to refine the default result.)

In BPMN the routing points (named waypoints there) are an important part of the model. It is not just the logical connection between two elements. And to be able to exchange the model (BPMN is of course a general standard) it is important to have this information in the model.
I agree with the idea form @martin-fleck-at to have at least some kind of PostUpdateAction to be able to update my model if I need this computed routing points from the client-side router algorithm. So maybe this can be an optional event which we evaluate in OpenBPMN and other tools can ignore this event.

Have you seen my second point in #730 that I would suggest to also re-calculate all routings after moving elements?

[planger]

Thank you very much for your feedback @rsoika! I get your points and we need to ensure that GLSP eventually supports those use cases.

Given that GLSP is a framework though (and not a concrete editor), I think we need to distinguish between whether the framework should do something (by default or optionally), or whether it should "just" enable adopters to do implement it by adding something on top. Since many of those things tend to be quite specific in the detail, I often tend to prefer the latter: enable adopters to implement either case with GLSP, but let it to the adopters of GLSP to implement it in detail.

The default route computed form the GLSP client should be fine for the most cases and of course I want to store this valuable information in my model. (In fact, I plan to invest more time in optimizing the route for Manhattan routing to refine the default result.)

I understand the reason that one may want to store the derived routes in the model (and not just the routing points manually set by the user), but I don't think it should be something that GLSP does or decides whether it be done or not. I know that many adopters of GLSP explicitly wouldn't want to store or even consider auto-computed routes as it is derived information and subsequent behavior is in certain editors different whether it is an auto-layouted routing point or a manually set routing point. For node sizes, it is a similar issue. Unless manually set, the size of nodes is usually not persisted, also to avoid redundancy and the need to propagate changes across the diagram (e.g. after label change that requires the containing node to become larger, requiring a shift in the center point to which an edge is anchored, etc.).

However, GLSP should certainly enable adopters who need to store or process this kind of information to do so. I suggest to let the glsp-client add the computed routing information to the ComputedBoundsAction. This feels consistent, as it addresses the same problem like the computed bounds (node sizes): it shares the values computed on the client with the server. The server can decide what it wants to do with it (store it somewhere, incorporate it in some handler logic, or ignore it).

In BPMN the routing points (named waypoints there) are an important part of the model. It is not just the logical connection between two elements. And to be able to exchange the model (BPMN is of course a general standard) it is important to have this information in the model.

Yeah, given that's the case for BPMN (and likely several other diagram types), GLSP should support that. But I think with my proposal above, GLSP would support that. BPMN could handle the set computed bounds action and transfer the routing points into the model state and eventually store it into the source model, similar to how it is also done for the node sizes, right?

I'm just a little bit wary to add a completely new generic mechanism with the PostUpdateAction for this purpose. I'm not opposed, but I would like to suggest re-using a mechanism that already exists and may address this requirement too.

Have you seen my second point in #730 that I would suggest to also re-calculate all routings after moving elements?

Yes, this is certainly a common feature. Some editors prefer to re-compute, for others manually set waypoints must stay in tact... this is really diagram-specific.
But I think, GLSP currently already supports that. Your ChangeBoundsActionHandler could just remove all routing points of all incoming and outgoing edges for each moved node. With that, the client-side routing starts from scratch. Or did I misunderstand your requirement?

[tortmayr]

I understand the reason that one may want to store the derived routes in the model (and not just the routing points manually set by the user), but I don't think it should be something that GLSP does or decides whether it be done or not. I know that many adopters of GLSP explicitly wouldn't want to store or even consider auto-computed routes as it is derived information and subsequent behavior is in certain editors different whether it is an auto-layouted routing point or a manually set routing point. For node sizes, it is a similar issue. Unless manually set, the size of nodes is usually not persisted, also to avoid redundancy and the need to propagate changes across the diagram (e.g. after label change that requires the containing node to become larger, requiring a shift in the center point to which an edge is anchored, etc.).

However, GLSP should certainly enable adopters who need to store or process this kind of information to do so. I suggest to let the glsp-client add the computed routing information to the ComputedBoundsAction. This feels consistent, as it addresses the same problem like the computed bounds (node sizes): it shares the values computed on the client with the server. The server can decide what it wants to do with it (store it somewhere, incorporate it in some handler logic, or ignore it).

I agree, handling this with an approach similar to the ComputedBoundsAction (or extending the ComputedBoundsAction) is probably the best solution for this problem. The default handler on the GLSP server would then simply update the GModel but bot the source model (similar to how we do it currently for dynamically computed shapes). Adopters are of course free to customize the handler as they see fit (e.g. persisting it in the source model, or completely ignoring it)

I'm just a little bit wary to add a completely new generic mechanism with the PostUpdateAction for this purpose. I'm not opposed, but I would like to suggest re-using a mechanism that already exists and may address this requirement too.

I agree, that the PostUpdaeAction might not be the best fit for this problem. However, in general I would be very much in favor of providing the possibility to queue actions on the client side and dispatch them after the next update. I think this is a very usefull feature and we already have a similar mechanism in place on the server-side (ActionDispatcher.dispatchOnNextUpdate)

Have you seen my second point in #730 that I would suggest to also re-calculate all routings after moving elements?

Yes, this is certainly a common feature. Some editors prefer to re-compute, for others manually set waypoints must stay in tact... this is really diagram-specific.
But I think, GLSP currently already supports that. Your ChangeBoundsActionHandler could just remove all routing points of all incoming and outgoing edges for each moved node. With that, the client-side routing starts from scratch. Or did I misunderstand your requirement?

I personally think it would be better if this would be configurable on the client side. E.g. that there is configuration flag that defines whether routes should be recomputed on move or not. This flag could even be set dynamically. The main advantage of this is that ensures that the client side feedback is consistent with final route after the server update. If we would only adapt the handling of the ChangeBoundsAction on the server side, the final route might be completely different compared to the client feedback.

In addition, we could also cover both usecases on a tool level. e.g. Normal moving-> keep existing route. Move + key press (e.g. shift) - >recompute route.

[planger]

The main advantage of this is that ensures that the client side feedback is consistent with final route after the server update. If we would only adapt the handling of the ChangeBoundsAction on the server side, the final route might be completely different compared to the client feedback.

Very good objection! I didn't consider client-side visual feedback. Since the client doesn't know what the server is going to do, the client will need to be made aware of whether the routes are going to be thrown away or not to provider proper feedback.

[rsoika]

Hi,
coming back to your discussion I want to ask you for a practical advice. I came to the point where it seems to be necessary to implement my own BPMN router (based on the Manhattan router). There are many details I am currently not happy with - the missing way points are only one. I want to refine the routing in general. Also the routing seems not to work in situations where the elements are within a container:

Can you please give me a jump start on how to go? I understand that the routing need to be done on the client. So should I start writing my own router and refer the new kind from my server code. Can you give me a short example how to start? At the end I understand I just need to register my own new router type on the server - right?:

public class SequenceFlowGNodeBuilder extends AbstractGEdgeBuilder<SequenceFlowGNode, SequenceFlowGNodeBuilder> {
  ...
    @Override
    protected void setProperties(final SequenceFlowGNode edge) {
        super.setProperties(edge);
        edge.setRouterKind(BPMNTypes.DEFAULT_ROUTER);
        ....
    }

Do you expect that I can solve the problems discussed here or are there more components I need to consider? I fear that the result will be very specific to BPMN and not have much potential for reusability. But I also don't want to go in the wrong direction....

[rsoika]

Hi,
I have now started to implement my own BPMNEdgeRouter based on the Sprotty Code of the ManhattanEdgeRouter.

To follow your thoughts in this discussion I figured out, that in the ManhattanEdgeRouter the method

route(edge: SRoutableElement): RoutedPoint[]

is that one I am interested in. Concrete line 60 is the moment where the client has what I am interested in.

So it looks not to complicated to me. It should be possible to send out the existing ChangeRoutingPointsOperation with the computed routing points to the server. Than I can easily update my BPMN source model.
Is this what you are talking about or did I misunderstand you? (Note: I have now my own implementation of a EdgeRouter which is part of my GLSP client. This is why I think I can use the ChangeRoutingPointsOperation here.)

The only thing which make me feel unsure is the fact, that the route() method (which is a complex piece of client code) is triggered also when I move the mouse over an existing un-selected edge. This feels wrong to me, because there is no reason to compute the route just because I am moving the mouse pointer across my diagram.

But anyway: please let me know if you all agree with my idea to send out the ChangeRoutingPointsOperation in this situation.

Again I have the big problem, that not a single line has been commented in the sprotty project. That makes it so damn hard to contribute to this project. I always feel like a fool asking only stupid questions....

[rsoika]

I figured out that when I send out a new action at the end of the route() method like this:

    route(edge: SRoutableElement): RoutedPoint[] {
       .........
       ......
        const action=ChangeRoutingPointsOperation.create([{ elementId:edge.id,newRoutingPoints:routedPoints}]);
        this.actionDispatcher.dispatch(action);
        return routedPoints;
    }

I get the server side ChangeRoutingPointsOperation event and I receive all the information I need.
But the dispatch call leads into an endless loop and the route() method is triggered again and again.....

[rsoika]

My final working solution looks now like this:

export class BPMNEdgeRouter extends AbstractEdgeRouter {
   .....
    route(edge: SRoutableElement): RoutedPoint[] {
        if (!edge.source || !edge.target) {
            return [];
        }
        // do we have already routing points or is this the first time..
        const initRoutingPoints=(edge.routingPoints.length===0);
        // compute route...
        ....
        ......

        // send a ChangeRoutingPointsOperation in case the routing points were computed the first time
        if (initRoutingPoints) {
          const action=ChangeRoutingPointsOperation.create([{ elementId:edge.id,newRoutingPoints:routedPoints}]);
          this.actionDispatcher.dispatch(action);
        }
        return routedPoints;
    }
   .....
}

As you can see, I test in the beginning if the edge Element has already routing points defined (init or update).
And I use the existing ChangeRoutingPointsOperation to sent the initial route back to the server. So it's up to the server if it is interested to the routing information or not.

But this works only because I am now implementing my own router. So the solution may be not usable for a generic solution...

[rsoika]

Sorry for polluting this thread but it is not working after all.
Finally - after a lot of debugging - I figured out that this all could be also a general problem of the Manhattan router.

The fact is, that the routing points computed by the Manhattan router, are never reaching the server. The anchor points (which should of course be part of the way-points) are never included in the ChangeRoutingPointsOperation. This makes it impossible for other tools to reconstruct the routing.

Since I just can't figure out who actually creates this event and when, it's not possible for me to work on the problem somehow. I give up frustrated at this point and wait until you are back from EclipseCon ;-)