Skip to content
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

Arrow visual and Bezier lines #1023

Closed
wants to merge 99 commits into from

Conversation

lrvdijk
Copy link

@lrvdijk lrvdijk commented Jul 27, 2015

This pull requests contains an extension to the already existing LineVisual: an ArrowVisual. This is a visual with an arrow head which is oriented automatically in the right way.

Supersedes these PRs:

Lucas van Dijk added 30 commits May 24, 2015 19:27
This module is ported from Glumpy, and contains code to generate
vertices for bezier lines.
The vispy.geometry.curves module provides several helper
functions to generate the right vertices for a nice curved
line.
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.
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)
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
This module is ported from Glumpy, and contains code to generate
vertices for bezier lines.
The vispy.geometry.curves module provides several helper
functions to generate the right vertices for a nice curved
line.
@larsoner
Copy link
Member

Thanks. We'll see what Travis thinks :)

FYI with this many commits you might end up seeing one Travis run fail due to size increases. That one we can selectively ignore in this case.

@rougier you have time to take a look at the code?

@larsoner
Copy link
Member

@sh4wn you need to update the test_data_tag = 'test-data-4' here:

https://github.com/vispy/vispy/blob/master/vispy/testing/image_tester.py#L364

Then hopefully your tests will pass.

@lrvdijk
Copy link
Author

lrvdijk commented Jul 28, 2015

Somehow a test fails to pass here: https://travis-ci.org/vispy/vispy/jobs/73004655

@larsoner
Copy link
Member

That's a weird one. Can you try setting up a 2.6 virtual environment to see if you can reproduce it? It's possible our raises decorator isn't working properly. You could try using assert_raises to see if it fixes it.

At the very least that exception could print out the exc itself like:

        else:
            raise AssertionError("Expected %s, got %s instead (%s)" %
                                (self.exc.__name__, type(exc).__name__), exc)

You also have some flake errors

@lrvdijk
Copy link
Author

lrvdijk commented Jul 29, 2015

Well, with Python 2.7 it works, don't known yet about 2.6 (Arch Linux doesn't package this version anymore).

I don't see the flake errors by the way?

Lucas van Dijk added 2 commits July 29, 2015 12:27
Somehow the raises contextmanager didn't work in Travis
and Python 2.6, so let's check with the `assert_raises`
function.
If the `raises` context manager gets an unexpected exception,
also print the exception string so the user can see what the
actual error is.
@larsoner
Copy link
Member

Maybe your version of PEP8 is old, see here for errors:

https://travis-ci.org/vispy/vispy/jobs/73166359#L1810

Can you rebase against the latest version of scenegraph-update? You then need to manually add Arrow to vispy/scene/visuals.py, we removed the auto-generation.

@lrvdijk
Copy link
Author

lrvdijk commented Jul 29, 2015

Oh damn, saw your comment too late, I used pull/merge. Rebasing always gives me a lot of merge conflicts and when I fix them and after I use git add it says "Nothing changed, have you used git add?".

@larsoner
Copy link
Member

Rebasing leads to a cleaner history, so in general it's what I prefer. It also makes things like squashing commits and removing files from history a bit easier if you ever have to do it. Whatever conflicts you get during rebase you should also hit during a merge, so I think it must just be a problem with how you're doing it. It might be worth reading up on a tutorial and learning how to do it in case you end up working with a package that is more strict about merges in the future. But we're okay here :)

Hopefully Travis is happy with this one.

@@ -228,6 +228,7 @@ def generate_docstring(subclass, clsname):
Isoline = create_visual_node(visuals.IsolineVisual)
Isosurface = create_visual_node(visuals.IsosurfaceVisual)
Line = create_visual_node(visuals.LineVisual)
Arrow = create_visual_node(visuals.ArrowVisual)
Copy link
Member

Choose a reason for hiding this comment

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

Was this file alphabetical until this change?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it almost was -- can you alphabetize at least the simple visuals? Compound we might actually want at the top of the entire list since it's a bit different from the others.

@lrvdijk
Copy link
Author

lrvdijk commented Jul 30, 2015

Cool, the tests pass now :)

@larsoner
Copy link
Member

LGTM, +1 for merge! The only Travis error is related to size, which is fine.

@campagnola I'm not sure what to do with this. It would be nice to have it merged into scenegraph-update so that future PRs by @sh4wn will have a nice diff, but I don't want to make merging the big PR more complex than it already is. I'm thinking if scenegraph-update won't be ready to merge for a couple of days, it's probably worth merging this one in now. WDYT?

This would otherwise result in an error.
@larsoner
Copy link
Member

@sh4wn can you close this PR and make it into master now instead? Then I can go ahead and merge, assuming Travis is happy.

@lrvdijk
Copy link
Author

lrvdijk commented Jul 31, 2015

Coming up!

@lrvdijk lrvdijk closed this Jul 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants