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

Fixed getPoint for same node edges #1907

Merged
merged 1 commit into from
Oct 17, 2016
Merged

Conversation

aanm
Copy link
Contributor

@aanm aanm commented Jun 14, 2016

If an edge was point out to the same node, the getPoint returns the wrong coordinates.

Signed-off-by: André Martins [email protected]

@aanm aanm force-pushed the fixing-get-point branch from f72904d to 944511f Compare June 16, 2016 00:48
@mojoaxel
Copy link
Member

@aanm It looks good but I can't figure out an simple example to test the changed getPoint function. Can you provide some code for testing this issue?

@aanm
Copy link
Contributor Author

aanm commented Aug 10, 2016

@mojoaxel Sure, can you please tell me where do you want me to put the test? Do you want it here or in a file under vis/test? If a file under vis/test which one?
Thanks

@mojoaxel
Copy link
Member

mojoaxel commented Aug 10, 2016

@aanm Most people create a jsbin or jsfiddle. If it's an short example just just post it here.

The code looks good but I want to test it before I merge it. What settings are needed that this function is executed? That's why I need an example.

@aanm
Copy link
Contributor Author

aanm commented Aug 10, 2016

@mojoaxel oh okay. so the problem is simple, I was following this thread #507 and when I used the code provided by @edvineshagh I saw that it didn't work for edges that were pointed to the same node. That's why I figure out the getPoints function had a bug for the edges that were pointed out to the same node. Can you test based on this?

@mojoaxel
Copy link
Member

Today I found the time to look into this again. I still don't get it. 😞

I tried to create an example with a node having an edge from and to itself. Also enabled default dynamic smoothing and arrows. As far as I can see the getPoint is never called on edges that have the same from and to. I can' test this.

@aanm Please provide a simple example that shows a use case for this pull request. Thx!

@aanm
Copy link
Contributor Author

aanm commented Sep 14, 2016

@mojoaxel There you go https://jsfiddle.net/2xc5r20j/ :-)
Like I said, the thread #507 is the one that calls the getPoint function. The animation code is a little buggy but the gist is there.
sorry I was planing on provide an example but I hadn't time to create it.

@yotamberk
Copy link
Member

@mojoaxel What's the status on this PR?

@mojoaxel
Copy link
Member

What's the status on this PR?

The code looks good but I hab problems creating an example to debug the changes. I think we can merge it, but I would feel better if I actually understood when this is needed.

@mojoaxel mojoaxel merged commit 8c42ab6 into visjs:master Oct 17, 2016
@mojoaxel
Copy link
Member

Shi**. This should not go into the master branch...I'll revert this.

@mojoaxel
Copy link
Member

@aanm Sorry, but I had to revert the changes because they were made to the master branch. If you still want them to be added please create a new PR to the develop branch. Thx.

@aanm
Copy link
Contributor Author

aanm commented Oct 17, 2016

@mojoaxel this way? #2161

@aanm aanm deleted the fixing-get-point branch October 17, 2016 14:34
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.

4 participants