New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bezier curves and arrows #1034

Merged
merged 107 commits into from Aug 3, 2015

Conversation

Projects
None yet
3 participants
@lrvdijk
Contributor

lrvdijk commented Jul 31, 2015

Add new vispy.geometry.curves module and an ArrowVisual.

lrvdijk added some commits May 24, 2015

Add geometry.curves module.
This module is ported from Glumpy, and contains code to generate
vertices for bezier lines.
Add an example how to draw Bezier curves.
The vispy.geometry.curves module provides several helper
functions to generate the right vertices for a nice curved
line.
Copy arrow shaders to the arrowheads folder.
These will contain modified shaders to be used with the ArrowVisual. In
contrast to the shaders in the vispy/glsl/arrows folder, do the shaders
in the vispy/glsl/arrowheads folder not draw the arrow body.
Update arrowhead shaders.
Change the following things:

- Only one color
- Remove option for head size
- The size option now represents the head size (we do not draw the arrow
  body)
Clean the GLSL shader code for arrow heads.
Things changed:

1. Use proper scaling when tranforming the coordinates in the fragment shader
2. Use correct coordinates when drawing the curved arrow heads
3. Add proper comment headings
4. Several other cleanups
Add geometry.curves module.
This module is ported from Glumpy, and contains code to generate
vertices for bezier lines.
Add an example how to draw Bezier curves.
The vispy.geometry.curves module provides several helper
functions to generate the right vertices for a nice curved
line.
Copy arrow shaders to the arrowheads folder.
These will contain modified shaders to be used with the ArrowVisual. In
contrast to the shaders in the vispy/glsl/arrows folder, do the shaders
in the vispy/glsl/arrowheads folder not draw the arrow body.
Update arrowhead shaders.
Change the following things:

- Only one color
- Remove option for head size
- The size option now represents the head size (we do not draw the arrow
  body)
@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 31, 2015

Member

Looks like you need another merge

Member

larsoner commented Jul 31, 2015

Looks like you need another merge

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Jul 31, 2015

Contributor

Pull from master?

Contributor

lrvdijk commented Jul 31, 2015

Pull from master?

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 31, 2015

Member

Yeah you have a conflict with master. I would normally say you should rebase but you have merges in there, so I think you have to merge again.

Member

larsoner commented Jul 31, 2015

Yeah you have a conflict with master. I would normally say you should rebase but you have merges in there, so I think you have to merge again.

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Jul 31, 2015

Contributor

It's able to merge now!

Contributor

lrvdijk commented Jul 31, 2015

It's able to merge now!

@campagnola

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jul 31, 2015

Member

No problem with rebasing after a merge. The only rule is: never rebase a branch that somebody else is working on.

Member

campagnola commented Jul 31, 2015

No problem with rebasing after a merge. The only rule is: never rebase a branch that somebody else is working on.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 31, 2015

Member

I usually get weird problems rebasing after merges because there are conflicts. I guess you could just skip those merge commits?

Member

larsoner commented Jul 31, 2015

I usually get weird problems rebasing after merges because there are conflicts. I guess you could just skip those merge commits?

@campagnola

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jul 31, 2015

Member

I wouldn't skip commits.

Member

campagnola commented Jul 31, 2015

I wouldn't skip commits.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 31, 2015

Member

How do you deal with the merge commits in when rebasing a branch that has merges in it? That's what I don't get... for example, in this branch there are multiple merges with a branch that is now in upstream/master, so it seems like you'd have to skip those merges during a rebase against upstream/master. What else would you do?

Member

larsoner commented Jul 31, 2015

How do you deal with the merge commits in when rebasing a branch that has merges in it? That's what I don't get... for example, in this branch there are multiple merges with a branch that is now in upstream/master, so it seems like you'd have to skip those merges during a rebase against upstream/master. What else would you do?

@campagnola

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jul 31, 2015

Member

If the merge really does not introduce any code changes, then it can be safely skipped (and in this case, I think git will give a warning and suggest skipping the commit).

That's not always the case, though--merge commits often contain extra changes that were not in either parent branch (especially if there were conflicts during the merge), so by skipping them you run the risk of missing these changes. The subsequent commits might also run into trouble because their changes might be applied against a different code base.

I haven't really had trouble with this, but in general my rule when picking between rebase and merge is to do whatever is easier :)

Member

campagnola commented Jul 31, 2015

If the merge really does not introduce any code changes, then it can be safely skipped (and in this case, I think git will give a warning and suggest skipping the commit).

That's not always the case, though--merge commits often contain extra changes that were not in either parent branch (especially if there were conflicts during the merge), so by skipping them you run the risk of missing these changes. The subsequent commits might also run into trouble because their changes might be applied against a different code base.

I haven't really had trouble with this, but in general my rule when picking between rebase and merge is to do whatever is easier :)

Show outdated Hide outdated vispy/visuals/line/arrow.py
'triangle_60',
'triangle_90',
'inhibitor_round'
]

This comment has been minimized.

@larsoner

larsoner Jul 31, 2015

Member

Let's make this a tuple so it's immutable

@larsoner

larsoner Jul 31, 2015

Member

Let's make this a tuple so it's immutable

Show outdated Hide outdated vispy/visuals/line/arrow.py
Enables or disables antialiasing.
For method='gl', this specifies whether to use GL's line smoothing,
which may be unavailable or inconsistent on some platforms.

This comment has been minimized.

@larsoner

larsoner Jul 31, 2015

Member

don't need this extra newline, same with a few others below

@larsoner

larsoner Jul 31, 2015

Member

don't need this extra newline, same with a few others below

Show outdated Hide outdated vispy/visuals/line/arrow.py
* angle_30
* angle_60
* angle_90
* inhibitor_round

This comment has been minimized.

@larsoner

larsoner Jul 31, 2015

Member

should indent these four more spaces

@larsoner

larsoner Jul 31, 2015

Member

should indent these four more spaces

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 31, 2015

Member

@campagnola there seem to be consistent errors in Travis for setting arrow.parent = None, any idea what is causing it? The ArrowVisual isn't doing anything fancy, so I think it's a deeper problem with the SceneCanvas:

https://travis-ci.org/vispy/vispy/jobs/73569457#L1851

Member

larsoner commented Jul 31, 2015

@campagnola there seem to be consistent errors in Travis for setting arrow.parent = None, any idea what is causing it? The ArrowVisual isn't doing anything fancy, so I think it's a deeper problem with the SceneCanvas:

https://travis-ci.org/vispy/vispy/jobs/73569457#L1851

[50, 75],
[75, 25],
[75, 75]
]).astype('f32')

This comment has been minimized.

@larsoner

larsoner Jul 31, 2015

Member

Cleaner to do ], dtype=np.float32) here instead of ]).astype('f32'). It also avoids a warning for old numpy.

@larsoner

larsoner Jul 31, 2015

Member

Cleaner to do ], dtype=np.float32) here instead of ]).astype('f32'). It also avoids a warning for old numpy.

@campagnola

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jul 31, 2015

Member

@campagnola there seem to be consistent errors in Travis for setting arrow.parent = None

No idea about this one. It looks like it's happening on Canvas.__exit__(). Might still be my fault, in any case.. Perhaps the multiple calls to c.render() are causing the framebuffer stack to be cleared incorrectly?

Member

campagnola commented Jul 31, 2015

@campagnola there seem to be consistent errors in Travis for setting arrow.parent = None

No idea about this one. It looks like it's happening on Canvas.__exit__(). Might still be my fault, in any case.. Perhaps the multiple calls to c.render() are causing the framebuffer stack to be cleared incorrectly?

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 31, 2015

Member

I tried to replicate with a Python2.6 venv on my system and couldn't, so this one might be hard to track down.

@sh4wn for now you can try just creating a new Canvas every time inside your loop, instead putting the loop inside the Canvas creation. If that works, please open up an issue with the trimmed stack trace from Travis, and a link to the Travis log.

Member

larsoner commented Jul 31, 2015

I tried to replicate with a Python2.6 venv on my system and couldn't, so this one might be hard to track down.

@sh4wn for now you can try just creating a new Canvas every time inside your loop, instead putting the loop inside the Canvas creation. If that works, please open up an issue with the trimmed stack trace from Travis, and a link to the Travis log.

lrvdijk added some commits Jul 31, 2015

Clean up a few small things in arrows.py
Improve documentation formatting a bit and change available arrow
types list to a tuple.
Create testing canvas inside the loop
Hopefully this stops some warnings when setting arrow.parent to
None.
@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Jul 31, 2015

Contributor

Ok I fixed those latest remarks :) Lets see what Travis thinks.

Contributor

lrvdijk commented Jul 31, 2015

Ok I fixed those latest remarks :) Lets see what Travis thinks.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 31, 2015

Member

Wow, that didn't fix it. I guess you can put it back to the more efficient single-creation way, but add this check:

if os.getenv('TRAVIS', 'false') == 'true' and sys.version[:3] == '2.6':
    raise SkipTest('Travis fails due to FB stack problem')  # XXX fix this
Member

larsoner commented Jul 31, 2015

Wow, that didn't fix it. I guess you can put it back to the more efficient single-creation way, but add this check:

if os.getenv('TRAVIS', 'false') == 'true' and sys.version[:3] == '2.6':
    raise SkipTest('Travis fails due to FB stack problem')  # XXX fix this
@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 31, 2015

Member

(and please open an issue about it)

Member

larsoner commented Jul 31, 2015

(and please open an issue about it)

lrvdijk added some commits Aug 3, 2015

Revert "Create testing canvas inside the loop"
This reverts commit cdbfc6c.
Creation of the testing canvas inside the loop dit not fix some
framebuffer stack problems with Travis.
Skip a few arrow tests on Travis
This is due a strange framebuffer stack problem.
@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 3, 2015

Contributor

Issue opened: #1042.

Contributor

lrvdijk commented Aug 3, 2015

Issue opened: #1042.

Move arrow_head property creation upwards
Create the arrow_head property of the ArrowVisual a bit earlier
before the object gets frozen.
@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 3, 2015

Member

LGTM! @sh4wn ready for merge from your end?

Member

larsoner commented Aug 3, 2015

LGTM! @sh4wn ready for merge from your end?

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 3, 2015

Contributor

Yes, I'm good to go!

Contributor

lrvdijk commented Aug 3, 2015

Yes, I'm good to go!

larsoner added a commit that referenced this pull request Aug 3, 2015

Merge pull request #1034 from sh4wn/bezier-arrows
Bezier curves and arrows

@larsoner larsoner merged commit d697a1a into vispy:master Aug 3, 2015

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Scrutinizer 2 new issues, 25 updated code elements
Details
continuous-integration/appveyor AppVeyor build succeeded
Details
@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Aug 3, 2015

Member

Great, thanks for working through this! It'll be useful to have this in. Now onto the graphs :)

Member

larsoner commented Aug 3, 2015

Great, thanks for working through this! It'll be useful to have this in. Now onto the graphs :)

@lrvdijk

This comment has been minimized.

Show comment
Hide comment
@lrvdijk

lrvdijk Aug 3, 2015

Contributor

Awesome!

Contributor

lrvdijk commented Aug 3, 2015

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment