Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

network - make negative scaleFactor reverse middle arrow correctly #3474

Merged
merged 1 commit into from
Sep 26, 2017

Conversation

MightyPork
Copy link
Contributor

This fix makes it possible to use negative scaleFactor (eg. -1) to flip the middle arrow.

The bug is with this line: 15 * scaleFactor + 3 * lineWidth - if scale factor is negative, lineWidth is applied in the wrong direction and the arrow gets smaller instead of larger.

Old behavior - wrong scaling when reversed (gets worse when selected):
screenshot_20170922_120556
screenshot_20170922_120606

New behavior - scales correctly
screenshot_20170922_120703

@MightyPork MightyPork changed the title Make negative scaleFactor reverse middle arrow correctly network - make negative scaleFactor reverse middle arrow correctly Sep 22, 2017
@wimrijnders
Copy link
Contributor

Hi there! Great of you that you are willing to contribute.

Can you please base the pull request on the develop branch?

Also, I'm not really sure how this actually reverses the arrow. I'll wait for the change to develop in order to check - but maybe you can explain.

@MightyPork MightyPork force-pushed the negative-arrow-scalefactor branch from 914a9f0 to 92b02ba Compare September 22, 2017 10:41
@MightyPork MightyPork changed the base branch from master to develop September 22, 2017 10:42
@MightyPork
Copy link
Contributor Author

hmmm it got all messed up when I rebased it to develop, maybe better to start over ??

It's really just the one line.

What reverses the arrow is not this change, but you using negative scale factor. But without my change, the scaling is wrong and the arrow becomes tiny when reversed.

@MightyPork MightyPork force-pushed the negative-arrow-scalefactor branch from 92b02ba to 01ea991 Compare September 22, 2017 10:45
@MightyPork
Copy link
Contributor Author

ah I think I fixed the rebase :)

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 22, 2017

Yes, please make a new PR and close this one. (Even if it is one line!)

Edit: Never mind, the rebase is good. I stand in awe that you managed that!

@MightyPork
Copy link
Contributor Author

no need now, did a hard reset to develop, cherry-picked my commit and force-pushed it

@wimrijnders
Copy link
Contributor

There is still enough room there for me to stand in amazement....

Copy link
Contributor

@yotamberk yotamberk left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Really awesome and elegant solution to the bug!

@yotamberk yotamberk merged commit 1fd58cc into almende:develop Sep 26, 2017
wimrijnders added a commit to wimrijnders/vis that referenced this pull request Sep 26, 2017
Addendum to almende#3474.

Updated the documentation, so that users can know about flipping the middle arrow with a negative scale factor.
Copy link
Contributor

@wimrijnders wimrijnders left a comment

Choose a reason for hiding this comment

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

Just one belated review comment: would you mind adjusting the docs for edges.arrows.middle, that users are aware that the direction of the middle arrow can be flipped by using negative values?

Oh, merged already.....never mind, I'll do it myself.

yotamberk pushed a commit that referenced this pull request Sep 29, 2017
* Network: Adjust documentation for arrows.middle.scaleFactor

Addendum to #3474.

Updated the documentation, so that users can know about flipping the middle arrow with a negative scale factor.

* Adjusted text for review
primozs pushed a commit to primozs/vis that referenced this pull request Jan 3, 2019
primozs pushed a commit to primozs/vis that referenced this pull request Jan 3, 2019
…3488)

* Network: Adjust documentation for arrows.middle.scaleFactor

Addendum to almende#3474.

Updated the documentation, so that users can know about flipping the middle arrow with a negative scale factor.

* Adjusted text for review
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants