Skip to content

Conversation

@aibcmars
Copy link
Contributor

@aibcmars aibcmars commented Apr 29, 2020

Closes #198

@tbouffard tbouffard changed the title 198 render parallel gateway [FEAT]render parallel gateway Apr 29, 2020
@tbouffard tbouffard changed the title [FEAT]render parallel gateway [FEAT] render parallel gateway Apr 29, 2020
@aibcmars aibcmars added the WIP 🚧 Pull request in progress and/or not ready for review. Used in addition to marking it as draft label Apr 29, 2020
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

@aibcmars I left some comments/questions.
I am testing locally the rendering and will attach screenshots when done.

}

protected paintInnerShape(c: mxgraph.mxXmlCanvas2D, x: number, y: number, w: number, h: number): void {
super.paintInnerShape(c, x, y, w, h);
Copy link
Member

Choose a reason for hiding this comment

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

❓ I see this super call in the 2 subclasses, could we remove it with the following

  • in the super class, move the code to the enclosing paintVertexShape method right before the paintInnerShape call
  • then we can remove the super calls
  • the paintInnerShape can then be made abstract

Copy link
Member

Choose a reason for hiding this comment

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

managed in f428cbc

c.setFillColor(this.stroke);
c.setStrokeWidth(0);
}
protected getScaledGeometry(x: number, y: number, w: number, h: number): { xS: number; yS: number; wS: number; hS: number } {
Copy link
Member

Choose a reason for hiding this comment

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

❓ good to share code! 😄
I have noticed that we didn't communicate well about the icon scaling and position of elements in the task (what I did) and gateway (what you did) shapes and we now have 2 ways of doing things 😯

I have introduced a MxScaleFactorCanvas class in charge of hidding the scaling

// vanilla canvas calls when a scale factor must be applied to positions
const scaleFactor = 0.26;
c.moveTo(8 * scaleFactor, 39 * scaleFactor);
c.lineTo(12 * scaleFactor, 25 * scaleFactor);

// MxScaleFactorCanvas
const canvas = new MxScaleFactorCanvas(c, 0.26);
canvas.moveTo(8, 39);
canvas.lineTo(12, 25);

This makes the code clearer than in the current gateway implementation. However, it only scales the x and y direction with the same value.

In the gateway side, you introduce a way to manage the translation required to position the icon. In the task shape, this is done globally by a translation of the current cursor which has a side effect: after icon drawing, the orgin coordinates have changed, so subsequent drawing won't be done at the right place, except if we remember to reset the origin coordinates at the end of the task icon drawing! For sure, this will create issue in the future when we will draw the task markers.

So to me, we have to merge the good part of the 2 implementations in a new single one, especially if we want to benefits of it for event icons.

I suggest that we keep the gateway icon implementation as it is now in this PR but we manage a refactoring very soon (let's say next week) with @csouchet as well. If you agree, I can create an issue to track this.

Copy link
Member

Choose a reason for hiding this comment

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

as discussed with @aibcmars, this will be managed in #226

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, done in 226 so resolving this conversation

Copy link
Member

Choose a reason for hiding this comment

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

not fully implemented in #226, so unresolving

@tbouffard
Copy link
Member

tbouffard commented Apr 29, 2020

@aibcmars can we change the size of the gateway icons

  • exclusive: it almost touches the enclosing rhombus
  • parallel: size ok but the width could be decreased a little

with commit a7bfff8
image

with master commit 90f3ccf
00_gw_parallel_rendering_implem_master_90f3ccf2

with #209 implementation
02_with_exclusive_gateway_rendering

this.putCellStyle(ShapeBpmnElementKind.GATEWAY_PARALLEL, style);
}

private configureGatewaysStyle(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

@aibcmars Can you remove unnecessary line style[this.mxConstants.STYLE_PERIMETER] = this.mxPerimeter.RhombusPerimeter; ?

Copy link
Member

Choose a reason for hiding this comment

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

can we please stop noticing this? we have to understand why this is put in all mxGraph examples
This seems to be ok without for our rendering but we do not know what is the purpose of this.
I suggest we manage this later and track this with a dedicated issue

What I have understood so far is that the perimeter provides the list of points that can be used to attach edges to the current shape. This could be usefull for automatic rendering.
I know we are supposed to have the waypoints in the BPMN files that should define the point on the shape where the edge must be attached, but without exactly knowing what is going on, it is to early to remove the perimeters from the style IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with Thomas, as yesterday stated: we have found it occurs in all mxgraph examples concerning RHOMBUS.

@aibcmars aibcmars removed the WIP 🚧 Pull request in progress and/or not ready for review. Used in addition to marking it as draft label Apr 29, 2020
@tbouffard tbouffard added enhancement New feature or request BPMN rendering Something about the way the lib is rendering BPMN elements labels Apr 29, 2020
@tbouffard
Copy link
Member

The gateway render is now ok with commit 0701745

image

@tbouffard tbouffard merged commit 9a22184 into master May 5, 2020
@tbouffard tbouffard deleted the 198_render_parallel_gateway branch May 5, 2020 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BPMN rendering Something about the way the lib is rendering BPMN elements enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEAT] Render Parallel gateway

4 participants