fix issue 140 wrong arrow angle

[yodalee]

#140

The steps to reproduce error:

var master = gitgraph.branch("master");
master.commit();
var branch0 = master.branch("branch0");
var branch1 = branch0.branch("branch1");
var branch2 = branch0.branch("branch2");
var branch3 = branch0.branch("branch3");

branch1.commit()
branch2.commit()
branch3.commit()

The arrow point from branch0 to branch2, branch3
will have vertical directions.

The cause:
There is a property 'isForkCommit' when drawing arrow
which if set to true, the direction of arrow will
re-calculate the vertical if difference in Y direction
is larger than Y spacing of template.
The direction of branch1 arrow is correct since
difference in Y direction is equal Y spacing of template.

The fix:
The property 'isForkCommit' seems unused.
Simply remove the property from calculation of arrow direction
can solve the issue.
However, more test on other cases are required to ensure that
removing isForkCommit property not affect other render.

[nicoespeon]

Thanks for helping @yodalee, that's really nice of you 👍

It sounds good, but I realized the project example doesn't illustrate these use cases.
I added 2 use cases so we can see the problem spotted in #140.

I pushed the commit on your branch so you can base your work on it: yodalee@d3723cc
(just run grunt server to see the graph in your browser)

Unfortunately, you can also see that this fix is causing the same trouble for another use case − which was using the isForkCommit variable:

Our issue here is that we don't handle the case of multiple branches created from a branch that has no commit. This case should behave just like if the origin branch had commits: the line should draw an angle, going straight to the commit − see picture above.

This happens because "the angle" only appears when commits of the other branches are "pushed" in Y because of the commit on the origin branch.
But this (= the angle) should also happen when the origin branch has no commit.

[yodalee]

Yes now there is use case, I can do more research on this problem.

[yodalee]

I think the problem lies in the branch line direction. In your case, the line become segment and a straight line in the empty branch case. Need to find why.

[yodalee]

I think I found a better fix to the problem, here is the commit.
The cause:
When create a new commit object, it will first check parentBranch
has commit or not. If it has commit, it will push a new point
to path, so there will have a broken-line to this commit.
In the above case, the branch0 has no commit when branch1, 2
commit, so branch1, 2 will have no breakpoint in path, instead
the line will straightly draw from master to the commit.

The fix:
The check of parentCommit is empty or not is actually not needed.
We can always draw broken-line by removing the condition.
Now every path to commit will have a broken point before it, and
the direction of arrow is correct now.
Also, it will push a point to parentBranch if parentBranch has commit.
The code will generate some error path. For example, in above case,
adding a branch0.commit() after branch2.commit()
The line will have not breakpoint in path since branch0 has a point
in it already when branch1 commit.

To be check:
I think there will have some problem if we adjust the template spacing.
Since the direction of arrow is vertical or not is determined by
the relative position of parent commit and current commit.
However the direction of path is now always segmented.
This acquires more example to check the correctness.

[nicoespeon]

Hi @yodalee!

I tested your proposal and it works nicely.
I found rendering correct, especially regarding what it looks like before your fix. I tried to change the template spacing and I've got issues with other parts of rendering way before having troubles with the segment + arrows here 😉

Thanks a lot for contributing 👍

[nicoespeon]

I spoke a little too quickly. After checking issue #140 I realized that there is a regression.

If you take this snippet:

var gitgraph = new GitGraph({
  template: "blackarrow",
});

var master = gitgraph.branch("master")
master.commit();

var branch0 = master.branch("branch0")
branch0.commit();
master.commit();
branch0.commit();

var branch1 = branch0.branch("branch1")
master.merge(branch0)

var branch2 = branch1.branch("branch2")
var branch3 = branch1.branch("branch3")
branch3.commit().commit();
branch2.commit();
branch3.commit();
branch2.commit();

The rendering is broken:

I re-open the issue and will wait to fix that point before releasing it.

[yodalee]

Yes we need some test case to verify that this modification is correct and will not cause other problem.

[nicoespeon]

I updated the example to illustrate the use case: 8f61ffc

I also reverted your change temporarily so I can release new versions of the graph (3d62564).

I keep #140 open until we figure out how to fix the issue without regression.