Reorganize visuals layer

[campagnola]

Reorganization to make visuals a top-level package. Fixes #448.
Only one example currently works; I'll deal with the rest after feed back on a couple of issues:

1. Any complaints about the current structure?
2. Poll: should transforms be top-level as well?
3. There are many modules for users to import, so it would be nice to pick one place where most of the common names are imported by default. Currently there is a bit of this going on in scene.__init__, but it's a mess. I'd like to make many of the examples look like:

import vispy.scene

canvas = vispy.scene.SceneCanvas()
line = vispy.scene.Line(...)
line.transform = vispy.scene.AffineTransform()

vispy.plot should do something similar, but with a more extensive set of imports aimed toward scientific vis.

[larsoner]

If we're going to have visuals be root-level as vispy.visuals, then shouldn't it be:

canvas = vispy.scene.SceneCanvas()
line = vispy.visuals.Line()
line.transform = vispy.transforms.AffineTransform()

or so? Unless there would be a difference between vispy.scene.Line and vispy.visuals.Line I don't see why we should allow both, seems more likely to be confusing than useful.

[campagnola]

The point of (3) is to avoid

import vispy.visuals
import vispy.scene
import vispy.transforms
etc

..but I'm not really attached to any particular answer. It's just something that should be settled before I rewrite the imports in the examples.

[larsoner]

Makes sense. I'm torn because I don't like having Line in both vispy.visuals.Line and vispy.scene.Line because it seems redundant. This makes me think that they should just stay in vispy.scene, but it's a bit strange to have everything collected there.

I guess it depends on how we want to think of vispy.scene. If we think of it as containing the machinery to connect components/visuals together in automated / useful ways, then it shouldn't really have Line in it, but should have things like SceneCanvas for example. If we think of it as the place where we collect all relevant high-level things across other sub-modules (e.g., vispy.transforms and vispy.visuals) to conveniently assemble them, then the current proposal (Line in two places) makes more sense to me. Is this what you had in mind?

I think I'm moving toward thinking everything should just stay in vispy.scene as it is now. I imagine the (vast?) majority of Visual users will use them with a SceneCanvas and its associated machinery, and those who are advanced enough not to do so, can be told in the docs that Visual objects can in principle be used without one.

[campagnola]

I think I'm moving toward thinking everything should just stay in vispy.scene as it is now. I imagine the (vast?) majority of Visual users will use them with a SceneCanvas and its associated machinery, and those who are advanced enough not to do so, can be told in the docs that Visual objects can in principle be used without one.

This is a good point. If we do want vispy.scene to be a "big" namespace, then it probably makes more sense to keep everything inside scene. I do like the idea of flattening scene out, though.. Another option is to avoid making scene a dumping ground in favor of some other top-level module, like vispy.all, or to simply not use big namespaces at all (although vispy.plot will probably end up being one).

Other opinions out there? So far this PR is about 5 minutes of effort, so I won't fuss if we decide to can it :)

[rossant]

Poll: should transforms be top-level as well?

same answer as the answer to "Can transforms be used without the scene graph?

Idem for visuals. If they can be used without the scene graph, let them be in vispy.visuals. I don't quite like the idea of a huge vispy.scene namespace.

In the short term, I'll probably be using visuals, transforms, and the shader chaining system for my work, but not the scene graph, so AFAIC it would make sense to have those things well-separated.

[campagnola]

The dependency graph looks something like this:

scene      => widgets, cameras, visuals, transforms
widgets    => visuals, transforms
cameras    => visuals, transforms
visuals    => transforms, shaders, gloo
transforms => shaders
shaders    => gloo

So there are many opportunities to break things up, but I'm not sure these all need to be top-level packages.

[larsoner]

I do like the idea of vispy.shaders being a repository of (potentially) reusable GLSL snippets. But it may or may not work that well for e.g. transformation shader code and other things that we want concurrent Python and GLSL code for, so we can think about that later.

[almarklein]

For Luke's point of the many import lines, it helps to write from vispy import scene, visuals.

Nevertheless, I also think we should keep scene as it is (except maybe for some internal refactoring). Visuals depend on stuff inside the scenegraph. Placing them in a separate subpackage is not going to make that go away.

We now found that having a separate subpackage would actually be confusing for most users. For users that only need visuals, I don't think from vispy.scene import visuals is so bad, is it?

[rossant]

Visuals depend on stuff inside the scenegraph.

Can you precise that?

Looking at Luke's dependency graph (but without having much knowledge about the details about the architecture), it would make sense to have:

scene
scene.cameras
scene.widgets

gloo
transforms
shaders
visuals

If we do this, nothing external depends on scene, which is what we want. For each one of gloo, transforms, shaders, and visuals, it makes sense to use them alone, without the scene graph. Having everything in a huge scene package seems like a bad design, because we'll end up with spaghetti code with everything depending on everything. Forcing ourselves to design transforms, shaders, and visuals not depending on the scene stuff will probably lead to a cleaner code base.

[almarklein]

I mean that Visuals depend on things in the scenegraph. Most importantly, Visual is a subclass of
Entity. Further it depends on transforms and shaders.

Although we could move transforms and shaders out of vispy.scene I am not even so sure about that. We would get the same problem that people need to import both scene and transforms. Maybe for shaders this is not a problem, because most users wont use them directly.

Having everything in a huge scene package seems like a bad design, because we'll end up with spaghetti code

I think that separating the visuals out, with vispy.visuals depending on vispy.scene, and the other way around, would be more spaghetti-ish than having them in a single package. What's wrong with from vispy.scene import visuals?

[rossant]

I mean that Visuals depend on things in the scenegraph. Most importantly, Visual is a subclass of
Entity.

What things specifically? Having visuals depend on stuff in the scene graph seems dangerous. From a logical standpoint, visuals should bundle GLSL code together to represent graphical objects. All visual objects should be independent. The scene graph is completely different: it gathers many visuals within a transform hierarchy. I see no logical reason for visuals to depend on stuff in the scene graph. It looks like a red flag to me...

Besides, @lcampagn's dependency graph does not support this fact that visuals depend on the scene graph... Maybe it is incomplete then?

I think that separating the visuals out, with vispy.visuals depending on vispy.scene

That's precisely the problem: visuals should not depend on scene (and I used this assumption, supported by Luke's graph, to propose the structure for the packages).

What's wrong with from vispy.scene import visuals?

It's not really a problem of import, it's more of a logical issue. The point of having subpackages is to decouple the functionality of a big project like Vispy into different components. Having too many dependencies between those components is bad design. Bidirectional dependencies are particularly dangerous, and I really think that the scene graph should depend on visuals, but not the other way around.

[almarklein]

Ok, so it seems that the core of the issue is that Visuals depend on the scenegraph, or specifically, the fact that Visual is a subclass of Entity.

Some history on that: early on we actually did have a separate subpackage for visuals and scenegraph. Entities were wrapping visuals in that approach. But this turned out to result in complex code and code repetition, which is why we refactored towards the current design.

Personally, I don't find this a problem. In part because most users will use the visuals in a scenegraph anyway. For other users (like you) importing the visuals from scene should not have any bad side-effects; the library of visuals will consist of much more code than the core of the scenegraph.

Maybe we should see vispy.scene as the package that defines the visuals, which happens to also implement functionality to use them inside a scenegraph. Come to think of it, we could consider renaming vispy.scene to vispy.visual.

[rossant]

Entities were wrapping visuals in that approach. But this turned out to result in complex code and code repetition

Yes I can understand that.

But I persist to think that it's a bad idea to have such dependency just for a minor technical issue (Visual needs to derive from Entity). IMO it would be worth thinking about how we can resolve this issue by keeping visuals "independent" from the scene graph.

• Why does Visual need to derive from Entity in the first place? What does Entity provide? Why is this subclassing critical for the scene graph to work? Can't we use some "duck-typing" or something..? (Python is not Java...)
• Could Entity be defined in vispy.visuals?
• Can we use some sort of mixin system so that Visuals do not derive from Entity, except when they're used in the scene graph (and without repeting code)?

[rossant]

Looking at the code if vispy.scene, I more and more think that it makes sense to uncouple visuals and the scene graph. Note that this is more from a logical standpoint rather than a practical one.

Here is how I see a Visual: "I give you a coordinate system, and you draw something".

The scene graph: "Maintain a hierarchy of coordinate systems (entities)"

Ideally, we would follow something similar to the entity component system (and I think that's actually where the Entity name comes from in the first place). The scene graph does nothing apart from handling the relationships between the entities. It offers a system where different components can "plug" into entities to provide functionality.

The visual component would plug into the transforms, and use that information to draw things in the scene.

Someone may want to implement a collision detection system: it would plug into the scene graph and use that information to detect colisions.

Separating visuals from the scene graph ensures that each system stays focused on its main task. The visual to draw stuff, the scene graph to maintain a hierarchy of entities.

Having visuals being tightly coupled to the scene graph would destroy the entity component design pattern, and it wouldn't make any sense to use entities at all then.

[almarklein]

Having visuals being tightly coupled to the scene graph would destroy the entity component design pattern

You have got a point there. Although I am not sure how strict we should follow that pattern. Maybe an example use case:

>>> line = vp.plot([1,2,3,1])  # Returns an Entity that wraps a Line Visual
>>> line.visual.line_color = 'r'  # mmm, long
>>> line.line_color = 'r'  # arg, need to replicate all visual properties.

# Also:
>>> line
<LineEntity at 0x876y78>  # arg, need to create an entity class for each visual
<Entity at 0x876y79>  # Generic entity that *has* a visual: arg, I cannot see it's a line
<Entity with visual=Line at 0x897978>  # Maybe something like this?

[rossant]

Can plot just return the Visual instead of the Entity?

[almarklein]

How would you then modify the transform or parenting etc?

[rossant]

line.entity? The idea being that when you create a Visual or another Component that depends on an entity, there's an entity property that returns it. I suppose most of the time, users will want to tweak visual's properties rather than entity properties.

[campagnola]

On Mon, Sep 15, 2014 at 4:47 AM, Almar Klein notifications@github.com
wrote:

I mean that Visuals depend on things in the scenegraph. Most importantly,
Visual is a subclass of
Entity. Further it depends on transforms and shaders.

In this PR, Entity, transforms, and shaders are all defined under visuals.
There should be no part of the visuals subpackage that imports anything
from scene.

[rossant]

In this PR, Entity, transforms, and shaders are all defined under visuals.

I would argue that shaders could be a top-level package (composition system + library of reusable GLSL snippets?). Not sure about transforms.

There should be no part of the visuals subpackage that imports anything
from scene.

+1

[almarklein]

Moving Entity along solves the problem, but it's still a bit awkward to have that class there...

@rossant Mmm, giving Visual an entity property seems pretty good actually ...

>>> line = vp.plot([1,2,3,1])  # returns a Line visual
>>> line.line_color = 'red'
>>> line.entity.transform = PolarTransform()  # much less common than modifying visual

I am starting to like this.

[rossant]

BTW, should all visuals have a transform property (useful either in the scene graph or outside it)?

[campagnola]

On Mon, Sep 15, 2014 at 8:04 AM, Cyrille Rossant notifications@github.com
wrote:

In this PR, Entity, transforms, and shaders are all defined under visuals.

I would argue that shaders could be a top-level package (composition
system + library of reusable GLSL snippets?). Not sure about transforms.

Sure; shaders and transforms can both be made top-level, or just shaders.
Entity remains the base class for all visuals. It's a little bit odd
because it provides the basis for hierarchical linking between entities,
but this actually does not depend on the scenegraph itself, so the
depepndency tree is not violated (and IMO, this is a better option than
mixin or multi-object solutions). We could even add a flag to prevent use
of the parent/child/transform attributes when visuals are used outside of
the scenegraph?

[campagnola]

On Mon, Sep 15, 2014 at 8:10 AM, Almar Klein notifications@github.com
wrote:

Moving Entity along solves the problem, but it's still a bit awkward to
have that class there...

@rossant https://github.com/rossant Mmm, giving Visual an entity
property seems pretty good actually ...

line = vp.plot([1,2,3,1]) # returns a Line visual
line.line_color = 'red'
line.entity.transform = PolarTransform()

This is interesting, but too verbose for such a commonly-accessed feature.
Perhaps when the entity is added, it also wraps its methods onto the
visual?

[almarklein]

BTW, should all visuals have a transform property (useful either in the scene graph or outside it)?

If they inherit from Entity (as is this PR) they should and the have. If they are entity components, then not, since the entity already provides this functionality.

[rossant]

Entity remains the base class for all visuals. It's a little bit odd
because it provides the basis for hierarchical linking between entities,
but this actually does not depend on the scenegraph itself, so the
depepndency tree is not violated (and IMO, this is a better option than
mixin or multi-object solutions). We could even add a flag to prevent use
of the parent/child/transform attributes when visuals are used outside of
the scenegraph?

Doesn't look so good to me... :(
Is it really complicated to have visuals not deriving from entity? This would make much more sense to me...

[campagnola]

On Mon, Sep 15, 2014 at 8:11 AM, Cyrille Rossant notifications@github.com
wrote:

BTW, should all visuals have a transform property (useful either in the
scene graph or outside it)?

All visuals in a scenegraph should have a transform. I am not sure it makes
much sense outside the scenegraph, though--in that context, you really want
to say visual.draw(doc_transform=..., render_transform=..., fb_transform=...)

[rossant]

Example of how things might work under the hood with the entity component system:

sg = SceneGraph()
v = Visual()
v.transform = ...
entity = sg.get_entity(...)
entity.add_component(v)  # link entity.transform to v.transform?

[almarklein]

Perhaps when the entity is added, it also wraps its methods onto the visual?

We could give the Visual a parent and transform property, but in that case we might as well just inherit from Entity :P This whole discussion is about the compromise between purity and practicality :)

[larsoner]

This fixed the Travis problem? I wish I knew how to do a confused+flabbergasted emoticon.

[campagnola]

Hey, I just noticed that Timer is never stored!

[larsoner]

This appears to have a number of good changes -- to make sure I understand, here's what I see:

1. Make vispy.scene.visuals into vispy.visuals with appropriate changes.
2. Auto-population of vispy.scene.visuals with the Visuals, with scene-nodeness added.
3. Change Entity to Node.
4. Better integration of dpi.

Is that right? Also, it looked like there were some changes to how config is used. Can you mention what those changes did?

Tests pass (other than size), and the changes look reasonable to me (other than my minor comments). It also looks like this was probably a PITA to get right -- thanks for tackling this! I'll let @almarklein and/or @rossant have a look before merge.

[almarklein]

Tests are passing; ready for final review / merge.

Awesome! I have a look first thing tomorrow morning!

[campagnola]

The changes I made to config are mostly just a reorganization of the module such that the config keys and CLI flags are all defined at the top of the module. This should make it easier to add new keys/flags in the future.

[larsoner]

Yeah this looks reasonable to me, I like it being private.

[almarklein]

maybe _to_shader_object() would make it even more clear?

[larsoner]

Crap, looks like there were conflicts with @almarklein's last PR, so this will need another rebase. Sorry about that :(

[larsoner]

Err I guess merge instead of rebase since you merged earlier.

[almarklein]

What about we append the docs of the visual, so that the user can see all possible arguments? Otherwise users must switch back and forth in the docs. The doc template could also be shorter IMO, and instead add some of this more detailed information in the module docstring.

[campagnola]

Docstring is now based on the original visual docstring with an extra note about VisualNode inheritance and extra args added to Parameters section.

[almarklein]

In examples/basics/scene/nested_viewbox.py preferred_clip_method should be clip_method. Also, setting the clip method to None gives wrong results. This can be left for another PR IMO.

In the mesh.py examples the spheres only take up the second half of the screen; the top half is just black.

Apart from this and my nitpick comments, this is looking good!

[coveralls]

Coverage decreased (-0.94%) when pulling d782335 on lcampagn:visuals-layer into 07dadee on vispy:master.

[almarklein]

wow, this apparantly harder then I thought, thanks for going through this :)

[campagnola]

Ready for merge, if there are no more comments?

[rossant]

+1 for me!

(side note: is the "size difference" test in Travis of any use??)

[almarklein]

Thanks a lot Luke! I especially like how we were able to converge to a solution that everybody is happy with. That's golden!

[campagnola]

@rossant: let's say a new developer adds a 100MB file to their topic branch and then deletes it right before making a PR. Who is actually going to notice that the repository just took on an extra 100MB of history? We have had this problem in the past and it is a huge ordeal to correct.

[rossant]

@lcampagn hmm makes sense!