Inconsistent spacing between arcs
Closed this issue · 23 comments
GoogleCodeExporter commented
What steps will reproduce the problem?
Difference between the two MSC's below (Note the change in order of the arcs in
the following MSC's).
msc {
a, b;
a => b [label="Single line"];
a => b [label="Two\nline"];
}
msc {
a, b;
a => b [label="Two\nline"];
a => b [label="Single line"];
}
What is the expected output? What do you see instead?
It is expected that spacing between arcs is consistent, but it is not. There is
space between arcs in first msc and no space in second msc. Also single line of
text is NOT centered vertically, whereas multi-line text is centered vertically.
Analysis:
Initially mscgen was designed to have constant arc height. But later when
multi-line text issue is fixed, arc height became variable, subjected to a
minimum height. This minimum height (arc spacing parameter in code) was
probably a requirement to retain the legacy view and spacing of msc's. It is
the source of the current issue.
Possible solution:
One way to fix the problem is to have a consistent/constant spacing between
variable-height arcs. By default we can have a non-zero spacing between arcs.
Remove the minimum height parameter in code and introduce the
SpacingBetweenArcs parameter. This spacing between arcs parameter shall be
controlled through as an msc attribute.
Using ||| is not a solution, because it is explicitly needed between every
multi-line arc or needed whereever there is a box and also spacing is still
inconsistent and NOT controllable.
With SpacingBetweenArcs parameter introduced, |||, ... can add more space
between arcs i.e. they serve as additional space arc. This additional space
amount introduced by ||| and ... can also be an attribute.
Original issue reported on code.google.com by v.arunmo...@gmail.com on 10 Sep 2010 at 9:38
- Blocked on: #50
GoogleCodeExporter commented
I've checked this, and the output I get is consistent with how it's always been
and is by design.
For a single row of text output, the text is just a above the arc line to
prevent the line type (dotted, dashed, double etc..) being obscured. To keep
multiple lines more compact and keep regular spacing of the lines (according to
the minimum height), a second row of text is then under the arc line.
In the case that >2 lines of text are used, then the strategy changes as more
space is needed between the arc lines to accommodate the text, so the line
becomes vertically centred on text.
So basically the arc line spacing will always be regular if each arc has <=2
rows of text. If you go above that, the spacing will expand as required.
So I don't think this is really a bug as such - just that you'd like it to
behave differently when using multi-row text? If that's the case, please
supply a an image of the problematic output (with markup if you like) and mark
this bug an enhancement.
Original comment by Michael....@gmail.com on 14 Sep 2010 at 9:23
Attachments:
GoogleCodeExporter commented
The main problem according to this bug is that there is no vertical gap between
the last line of text of previous arc and the first line of text of next arc.
If the previous arc has one line of text, there is enough vertical gap to the
next arc. If the previous arc has two lines of text, it looks as if the
vertical gap to the next arc is much smaller than in the first case. If the
previous arc has >2 lines of text, there seems to be no gap at all. Here the
term gap implies vertical white space.
I would like to have a constant look and feel of arcs regardless of number of
lines of text and also I would like to avoid introducing ||| unnecessarily.
Regarding alignment of text vertically, all the line arcs can have the text
completely above the line regardless of single or multi-line text (As you told
this will NOT obscure the line). For a box arc, the text can be within the box
as it is now.
Regarding white space between arcs, every arc should be followed by
configurable amount of white space. As we can see from the attached pdf, every
arc is followed by a 15pt vertical space. Attached pdf is the desired ouput of
the following msc.
msc {
A [label="Entity A\n(Entity Info)"], B [label="Entity B"];
A => B [label="Single Line Text"];
A << B [label="Two Line\nText"];
A <<= B [label="Three\nLine\nText"];
--- [label="Multi\nLine\nText\nin\nDivider"];
A box B [label="Single-line inside box"];
A abox B [label="This\nis\nmulti-line\ninside\nbox"];
... [label="Space generated by ..."];
A -> B [label="Four\nLines\nof\nText"];
||| [label="Space generated by |||"];
}
Original comment by arunmozh...@gmail.com on 17 Sep 2010 at 5:31
Attachments:
GoogleCodeExporter commented
Attached png is what is currently generated by mscgen
Original comment by arunmozh...@gmail.com on 17 Sep 2010 at 5:36
GoogleCodeExporter commented
Is there a way to change this bug to enhancement. I am using firefox and I do
not find an option to do the same :-(. Please change this bug to enhancement.
Original comment by arunmozh...@gmail.com on 17 Sep 2010 at 5:55
GoogleCodeExporter commented
Original comment by NThykier@gmail.com on 17 Sep 2010 at 11:13
- Added labels: OpSys-All, Type-Enhancement
- Removed labels: Type-Defect
GoogleCodeExporter commented
I can point to some location in code which might be the source of the issue. In
the codebase of version 0.18, lines 596 and 1469 has the following code
ymax = ymin + gOpts.arcSpacing;
if(arcLabelLines > 1)
{
ymax += (arcLabelLines - 2) * textHeight;
}
I am doubting that "arcLabelLines - 2" is probably some hack to center the text
vertically. Probably this should be "arcLabelLines - 1", because for the first
line of text gOpts.arcSpacing has been added and for each further line
textHeight should be added. "arcLabelLines - 2" implies we are removing
vertical white space of one textHeight provided by gOpts.arcSpacing.
I do not have Cygwin or linux to verify if this is the case. May be someone can
try this out.
Original comment by arunmozh...@gmail.com on 17 Sep 2010 at 3:35
GoogleCodeExporter commented
Here is the output from changing arcLabelLines - 2 into arcLabelLines -1.
Note that this also affects test7; I have attached those as well.
Original comment by NThykier@gmail.com on 17 Sep 2010 at 4:15
Attachments:
GoogleCodeExporter commented
Hmm... at a first glance it looks like -2 for boxes and -1 for others might
make sense. Anyhow this appears very related to issue 50, so marking this
dependent on 50.
Original comment by NThykier@gmail.com on 17 Sep 2010 at 4:37
GoogleCodeExporter commented
[deleted comment]
GoogleCodeExporter commented
Hi NThykier, Thanks for the images. With these images, we can conclude that
"arcLabelLines - 2" is incorrect.
Assuming we change it to "arcLabelLines - 1",
Total Vertical Space allocated for an arc = gOpts.arcSpacing + ((Number Of Lines of Text - 1) * textHeight)
Out of this vertical space, White Space available = gOpts.arcSpacing - textHeight - any line thickness of line arc or box arc
The following are the observations
(1) Every line arc is centered within the vertical space allocated for the arc.
(2) Every box arc is drawn occupying the complete vertical space
(3) Text is always drawn top aligned except for few cases where it is shifted down a little to give an impression of center alignment. This hack is at line 803 in main.c (y += textHeight / 2)
(4) Divider text looks vertically centered because the divider line is drawn only when rendering middle line of the text.
The following changes are needed to keep the spacing and alignment consistent
(1) Meaning of gOpts.arcSpacing configuration is not consistent in code currently. Instead a more appropriate configuration gOpts.verticalGap can be used. With this change,
Total Vertical Space allocated for an arc = gOpts.verticalGap + (Number Of Lines of Text * textHeight) + any line thickness of line arc or box arc
Out of this vertical space, White Space available = gOpts.verticalGap
(2) For line arcs, text can be above the line. So the vertical components of line arc are Single/Multi-line Text followed by line followed by white space (gOpts.verticalGap).
(3) For divider, text can be vertically centered. So the vertical components of divider are Single/Multi-line Text followed by white space (gOpts.verticalGap). Divider line is drawn in the vertical middle of text as it is now and so it does not occupy any additional vertical space.
(4) Space provided by ... & ||| has the text vertically centered. In this case the total vertical space occupied will be MAX(Total Text Height, gOpts.minimumSpace) + gOpts.verticalGap.
(5) For box arcs, text will be within box. So the vertical components of box arc are top line of box followed by Single/Multi-line Text followed by bottom line of box followed by white space (gOpts.verticalGap).
(6) In case of curved line arc, text is left/right aligned with the top of the text aligned to the top of the arc. In this case the total vertical space occupied will be MAX(Total Text Height, ellipse minor axis length) + gOpts.verticalGap.
Original comment by arunmozh...@gmail.com on 18 Sep 2010 at 9:35
GoogleCodeExporter commented
Original comment by Michael....@gmail.com on 4 Oct 2010 at 8:59
- Changed state: Accepted
GoogleCodeExporter commented
I can give patch for the problems 44, 46, 48 and 50 in one or two days for you
to verify.
Original comment by arunmozh...@gmail.com on 7 Oct 2010 at 3:01
GoogleCodeExporter commented
SVN patch containing the fix for the issues 44, 46, 48, 50 attached.
Changes Summary
(1) A bottom-up level iterator for MSC has been added. This is required for the
calculation of size of levels bottom-up. This approach takes care of arc skips.
(2) Function to calculate the arc span along Y direction is added. There are
mainly two output values of the function: span above mid and span below mid.
These two variables allows to align parallel arcs along their middles.
(3) Computation of canvas size modified and information related to each level
(size of each level) is stored during the computation. This is required for arc
skip computation.
(4) Line/Box/Tex drawing functions do not change much. Only the parameters with
which they are invoked changes in main().
Original comment by arunmozh...@gmail.com on 8 Oct 2010 at 1:41
Attachments:
GoogleCodeExporter commented
Same patch merged with the current SVN head revision as there were some
conflicts.
Original comment by v.arunmo...@gmail.com on 12 Oct 2010 at 2:28
Attachments:
GoogleCodeExporter commented
Thanks for the patch.
I took a look at it, and took some of the ideas for issue #50 which is now
fixed. The code produces a lookup of row heights (or 'shelves' in your
parlance) which simplifies the layout one the 2nd pass.
These issues are still open however!
Original comment by Michael....@gmail.com on 17 Oct 2010 at 10:32
GoogleCodeExporter commented
I do not understand what you mean by issues are still open. Did you apply this
patch correctly? I have attached the pictures taken after the attached patch
got applied.
If you find any issues, please let me know.
Original comment by v.arunmo...@gmail.com on 18 Oct 2010 at 2:28
Attachments:
GoogleCodeExporter commented
Sorry - to clarify, I've not taken the patch. Instead I fixed issue 50 in a
different way, and will subsequently work on the remaining open issues.
Original comment by Michael....@gmail.com on 19 Oct 2010 at 8:03
GoogleCodeExporter commented
Not a problem. Hope u will come to the same logic used in the patch ;-). Just
that I wanted to make sure that my expectations are met.
Original comment by arunmozh...@gmail.com on 20 Oct 2010 at 1:31
GoogleCodeExporter commented
This issue was closed by revision r161.
Original comment by Michael....@gmail.com on 26 Oct 2010 at 8:17
- Changed state: Fixed
GoogleCodeExporter commented
Thanks Michael for fixing the issue. Some minor issues still remain.
(1) The problem mentioned in the first msc's of the bug still remain (See the
initially attached b1.msc.png & b2.msc.png). That is, Space is more following
a line arc with single line of text.
Don't you think the following code is a hack to make sure enough space is available above a line arc with a single line of text? (See the 2 in the code below). It introduces more space below the line arc with single line of text.
main.c:870: ymax = ymin + gOpts.arcSpacing;
main.c:871: ymax += (M_Max(rowHeight[row].maxTextLines, 2) * textHeight);
The problem will be much more obvious if a font of higher size is used.
(2) What is the necessity to vertically center the text of line arcs having
text more than 2 lines? You had already commented that this will obscure the
arc itself. Hope you did not think that there is a compatibility issue. Nobody
cares where label is positioned, as there is no explicit control/documentation
given in previous revisions.
The basic issue is that you have aligned the parallel arcs assuming that the
span is equal above and below the mid line. It is required to consider these
two spans different as mentioned in point 2 of the patch above. Not only it
solves the above problems, it will help later when labels of arc skipped line
arcs is positioned and when introducing new arcs.
You already seemed to have introduced the required variables, ymin, ymid and
ymax. Just that the ymid need not be (ymin + ymax)/2.
Original comment by arunmozh...@gmail.com on 27 Oct 2010 at 2:30
GoogleCodeExporter commented
Hi arunmozhi.v,
There's no hacks ;)
The spacing is as per the original design whereby each arc always has space for
1 line of text above the arc line and 1 line below it. This keeps the spacing
regular in the case that each arc has less than 3 lines of text. If an arc has
more than 2 lines of text, those lines get vertically centered on the line.
This harks back to the original mscgen which only allowed 1 line of text per
arc.
The change now is that there is also a small gap left between each row to
prevent lines of text on the bottom of one arc rolling into the first line of
the next arc. This is present very consistently, whether using arcs or boxes.
> (2) What is the necessity to vertically center the text of line arcs having
text more than 2 lines?
Given an even number of lines, you get half above and half below the arc. I
think that's neater and uses less space than putting them all above the arc.
> Nobody cares where label is positioned,
So did you check with everybody ;-D
Original comment by Michael....@gmail.com on 27 Oct 2010 at 7:07
GoogleCodeExporter commented
Hi Michael, I do not agree with the explanation you have given. There is
clearly a problem that you do not want to appreciate. The problem is that, the
text obscures the arc, if vertically centered. If arc line is not visible, what
is the use of text (See the output of testinput17.msc)? Does the Mscgen has
responsibility to make sure there is no obscuring or overlapping? By default
Mscgen should make sure that the arcs are not obscured and if required, there
should be a valign=<top or center or bottom>, halign=<left or center or right>
and textpos=<0 to 1> attribute which the user can use to position the text at
his will. It is the user who should decide which positioning will make the MSC
neater, and the tool should provide options. It is also the responsibility of
tool to make it as much compact as possible, but not at the cost of something
else.
Secondly, there is no reason to make sure that the spacing is regular when the
the number of lines of text is below 2 and irregular when above 2 lines of
text, just because it was like this before. You see it as a regular spacing,
but I see it as irregular vertical gaps. If required, this spacing regularity
might be provided through a separate user configurable variable e.g. Minimum
arc height and by default it can be configured to have the existing behavior.
> So did you check with everybody ;-D
I checked with your documentation which everybody reads i.e. I meant the change
did not conflict with the documentation in any way.
Only by making sure that there are two variables (above span and below span),
it is possible to provide any kind of vertical alignment and that is the
difference between the patch and you design. I do not want to interfere in your
design decisions, but just wanted to rationalize what I have done. It should be
accepted that it is required for solving other problems and future enhancements
(e.g. inline expression/frames, arc skips/gradients etc.).
Original comment by arunmozh...@gmail.com on 28 Oct 2010 at 4:47
GoogleCodeExporter commented
Thanks for your comments, and your contributions.
I think we have a difference in vision here as I'm happy with the changes I've
made and don't feel the need to make further alterations in this area at
present. Most productive now is probably to mop up any regressions and corner
cases and then push for the next release.
Original comment by Michael....@gmail.com on 2 Nov 2010 at 10:11