WIP: RectVisual, Regular polygon visuals, and more.

[mssurajkaiga]

As a start, I have updated the EllipseVisual to support float input as radius to draw a circle. I have also replaced instances of paint with draw which wasn't done in PR #275

[campagnola]

Try to avoid repeated code as much as possible. For example, set_gl_options is called once for each constructor, which means it is called 3 times for the RegularPolygon constructor.

[campagnola]

Sorry, hadn't realized this had been updated.

[campagnola]

I think this is ready for merge; it just needs to be merged with scenegraph first to resolve a minor conflict.

[mssurajkaiga]

Sure. No problem. I'm currently working on tests for the EllipseVisual.
Also, could you explain more on the difference b/w document coordinate
system and pixel coordinate system?

With regards,
M S Suraj

On Sat, Jul 5, 2014 at 12:22 AM, Luke Campagnola notifications@github.com
wrote:

I think this is ready for merge; it just needs to be merged with
scenegraph first to resolve a minor conflict.

—
Reply to this email directly or view it on GitHub
#284 (comment).

[campagnola]

On Fri, Jul 4, 2014 at 3:12 PM, M S Suraj notifications@github.com wrote:

Also, could you explain more on the difference b/w document coordinate
system and pixel coordinate system?

I recommend to read up on the difference between logical pixels and
physical pixels, and then read the recent additions to the transform
thread. Feel free to join that discussion if you have further questions.
We also discussed lpx/dpx some time ago:
#99 (comment)

[mssurajkaiga]

Thanks.

With regards,
M S Suraj

On Sat, Jul 5, 2014 at 6:28 AM, Luke Campagnola notifications@github.com
wrote:

On Fri, Jul 4, 2014 at 3:12 PM, M S Suraj notifications@github.com
wrote:

Also, could you explain more on the difference b/w document coordinate
system and pixel coordinate system?

I recommend to read up on the difference between logical pixels and
physical pixels, and then read the recent additions to the transform
thread. Feel free to join that discussion if you have further questions.
We also discussed lpx/dpx some time ago:
#99 (comment)

—
Reply to this email directly or view it on GitHub
#284 (comment).

[mssurajkaiga]

@lcampagn Here's a sample test that uses a reference image and checks if the visual data is same as that. I was thinking of adding a diff based comparison with some tolerance level for other tests. Since vispy.util.dataio does not have an image comparison function, I think it would be appropriate if I add it there. WDYT?

Also, a373cd4 adds requires_img_lib decorator which is not in scenegraph branch. If available, it would remove out a few lines of code from future visual tests.

[larsoner]

If the goal is to use it for testing, my vote would be to put it in vispy.testing so we can just do something like:

from vispy.testing import assert_image_almost_equal

Assuming we can settle on a reasonable API for the function, this would work well. I had in mind checking the maximal value of a cross-correlation, since this would allow for small shifts/inaccuracies. But we might have to experiment with different ideas to get it to work.

Note that a diff-with-tolerance is exactly what from numpy.testing import assert_allclose and assert_array_almost_equal are both designed for, so if that's "good enough" for now, just use one of those (I prefer assert_allclose, it's more flexible).

Regarding the requires_img_lib, that can be settled the next time @lcampagn does a merge with upstream -- I'd leave it separate / duplicated for now.

[campagnola]

Note: image data should not be added to the main repository. For this PR, you should backtrack to the last commit before the image was added, and re-commit from there without the image. I have opened a new vispy/test-data repository that you can push test data into, and the test should attempt to download the test data from there if it is not already available. (see https://github.com/vispy/vispy/blob/master/vispy/util/_data.py#L21, perhaps that can be modified to facilitate this?) @eric, perhaps we should discuss a basic system for enabling this kind of testing throughout vispy.

[larsoner]

@lcampagn that should be pretty easy. We already have functions for downloading data for examples, should be straightforward to adapt that to download test data (and save to disk permanently) on demand.

[campagnola]

I wonder if, for testing purposes, it might be better to just use git to synchronize the complete set of test results? Then when we run make test it could do a pull to check for new images immediately before testing.. We run into a weird issue, though, where tests that used to pass on one commit will no longer pass if the test data is updated..

[campagnola]

Perhaps we should continue this discussion over at #89 .

[campagnola]

Strike that, #156 is closer.

[mssurajkaiga]

@lcampagn My bad. I will re-commit.
@Eric89GXL For now I will try tests with assert_allclose but as more visuals and features get added, it would be good to have an API for better image comparison algorithms.

[mssurajkaiga]

@lcampagn Can this be merged? I think it would be better if I wrote all the tests in a different PR.

[campagnola]

Agreed.
However, can't merge yet--the image is still in the commit history. Reverting and adding new commits does not remove the prior commits (the point is to minimize growth of the repository over time, so we try to make sure images are not added anywhere in the history). The last commit without the image was 3c00645, so I think this should work:

git checkout 3c00645
git checkout -b more-visuals-clean
git checkout more-visuals ./
git commit
git push -f origin more-visuals-clean:more-visuals

Normally push -f is dangerous, but in this case we know that nobody else has based new work on the commits that will be forced out.

[larsoner]

You could also use this if you want:

$ git checkout more-visuals-clean
$ git rebase -i HEAD~2  # during this interactive rebase, delete the offending commit(s)
$ git push origin --force more-visuals-clean:more-visuals

git usually provides a few different options for these sorts of things.

[campagnola]

Grazie!

[campagnola]

Request: for a future PR, can we add properties like Ellipse.radius, Polygon.border, Polygon.fill_color, etc? Ideally, changing any of these properties should call Visual.update() to inform listeners that the visual needs to be redrawn, and should delay rebuilding the geometry until Visual.draw() is called.

[mssurajkaiga]

Sure. I will keep it in mind.
Also, dumb question, I'm not able to fork the 'test-data' repo. Is this an issue from github end?