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

Scenegraph overhaul #928

Closed
wants to merge 290 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@campagnola
Member

campagnola commented May 25, 2015

This PR is a continuation of #895 (and supersedes it). I am keeping both PRs open for now so that the prior changes may be reviewed in isolation. Major tasks for this PR are:

  • Remove multi-parenting in scenegraph
  • Nodes have a single transform (and related simplifications)
  • SceneCanvas.draw_visual should be simple and efficient
  • Implement picking (finally)
  • ViewNode and a way to automatically create a view on a scenegraph subtree
  • Other simplifications:
    • Remove all ViewBox clip methods except fragment.
    • Remove render/framebuffer/canvas scenegraph nodes (might restore these later if there turns out to be a need)

Visuals to migrate:

  • Line
  • Mesh
  • GridLines
  • Markers
  • Volume
  • Image
  • Text
  • Axis
  • Polygon
  • Box
  • Cube
  • Ellipse
  • Isocurve
  • Isosurface
  • Plane
  • Rectangle
  • RegularPolygon
  • Spectrogram
  • Tube
  • XYZAxis
  • Histogram
  • LinePlot
  • Isoline
  • SurfacePlot
  • ColorBar

EDIT BY Eric89GXL:

Related issues:

  • Toggling a node toggles children #986
  • Text visual changing viewport #981
  • Type of Canvas size #976
  • Centering of text visuals #975: working now?
  • Scene benchmarks broken #965: need to fix?
  • Depth drawing precision #953: need to add?
  • Get SceneCanvas from node #927: exists?
  • Plane and box PR #901: need to update visuals
  • Original PR #895
  • Reuse FBO in viewbox #791
  • Picking example #693
  • Axes, ticks, grid #677
  • Grid plot with individual zooming #434: does it work?
  • Picking #140
  • Line editing example PR #926: modify to use picking?
  • Camera translate speed #914: need to add?
  • Performance of the SceneGraph #628

closes #986
closes #981
closes #976
closes #975
closes #965
closes #953
closes #927
closes #791
closes #693
closes #677
closes #434
closes #140

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner May 28, 2015

Member

@campagnola can you add the transform from #884 while you're digging around in there? Also, does this obviate the need for #791?

Member

larsoner commented May 28, 2015

@campagnola can you add the transform from #884 while you're digging around in there? Also, does this obviate the need for #791?

@larsoner larsoner added this to the version 0.5 milestone May 28, 2015

@campagnola campagnola referenced this pull request May 28, 2015

Closed

Reuse FBO in viewbox #791

campagnola added some commits Jun 3, 2015

@larsoner larsoner referenced this pull request Jun 3, 2015

Merged

Fix type of canvas size. #976

@campagnola

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jun 4, 2015

Member

I think at this point I am ready to migrate the visuals to the new system. If anyone would like to volunteer help with this, just let me know which visuals you are working on and open a sub-PR. MeshVisual is already migrated, so I would start with that as an example the changes that need to be made.

Member

campagnola commented Jun 4, 2015

I think at this point I am ready to migrate the visuals to the new system. If anyone would like to volunteer help with this, just let me know which visuals you are working on and open a sub-PR. MeshVisual is already migrated, so I would start with that as an example the changes that need to be made.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jun 4, 2015

Member

@bollu can you take up one of these and see how feasible it is? It would be a great way to learn how the new system works. If it's straightforward maybe you could do many of them. Otherwise I can help.

Member

larsoner commented Jun 4, 2015

@bollu can you take up one of these and see how feasible it is? It would be a great way to learn how the new system works. If it's straightforward maybe you could do many of them. Otherwise I can help.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jun 4, 2015

Member

BTW @campagnola what is the idea behind the ViewNode? When would it be used?

Member

larsoner commented Jun 4, 2015

BTW @campagnola what is the idea behind the ViewNode? When would it be used?

@campagnola

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jun 4, 2015

Member

ViewNode is just the scenegraph equivalent of VisualView. Example:

# Show a PlotLine in two different ViewBoxes
c = SceneCanvas()
g = c.central_widget.add_grid()
v1 = g.add_view(0, 0)
v2 = g.add_view(0, 1)
plot = PlotLine(data, parent=v1)
plot_view = plot.view(parent=v2)  # this is a ViewNode
Member

campagnola commented Jun 4, 2015

ViewNode is just the scenegraph equivalent of VisualView. Example:

# Show a PlotLine in two different ViewBoxes
c = SceneCanvas()
g = c.central_widget.add_grid()
v1 = g.add_view(0, 0)
v2 = g.add_view(0, 1)
plot = PlotLine(data, parent=v1)
plot_view = plot.view(parent=v2)  # this is a ViewNode
@bollu

This comment has been minimized.

Show comment
Hide comment
@bollu

bollu Jun 4, 2015

Contributor

@Eric89GXL you mean the open issues related to this change, correct? And sure, I'll take a look at them :)

Contributor

bollu commented Jun 4, 2015

@Eric89GXL you mean the open issues related to this change, correct? And sure, I'll take a look at them :)

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jun 4, 2015

Member

@bollu by "one of these" I mean one of the visuals @campagnola wants help translating. Basically this branch changes a lot about how visuals work (simplifying and streamlining), so we need to change over all of our visuals to the new system. Can you try translating some of them?

Member

larsoner commented Jun 4, 2015

@bollu by "one of these" I mean one of the visuals @campagnola wants help translating. Basically this branch changes a lot about how visuals work (simplifying and streamlining), so we need to change over all of our visuals to the new system. Can you try translating some of them?

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jun 4, 2015

Member

@campagnola I edited PR description to contain all the PRs and open issues that have been related to these changes. I'm not sure of the status of all of those things, but some of them (e.g., adding parameters to cameras) one of us can take care of. @bollu if you want to start there instead of diving into Visual translation that's also a good option.

Member

larsoner commented Jun 4, 2015

@campagnola I edited PR description to contain all the PRs and open issues that have been related to these changes. I'm not sure of the status of all of those things, but some of them (e.g., adding parameters to cameras) one of us can take care of. @bollu if you want to start there instead of diving into Visual translation that's also a good option.

Merge branch 'multiprogram' into scenegraph-update
Conflicts:
	vispy/visuals/visual.py

@larsoner larsoner referenced this pull request Jun 5, 2015

Closed

Vispy plotting API design #918

Merge remote-tracking branch 'demo/demos' into scenegraph-update
Conflicts:
	examples/demo/scene/flow_lines.py
	examples/demo/scene/oscilloscope.py
@campagnola

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jul 14, 2015

Member

I'm pushing, but the bugs keep coming ;)

Member

campagnola commented Jul 14, 2015

I'm pushing, but the bugs keep coming ;)

@bollu

This comment has been minimized.

Show comment
Hide comment
@bollu

bollu Jul 14, 2015

Contributor

@campagnola is AGG ported? the AGG example seems to be working for me right now. Or is it using some other anti-aliasing technique?

Contributor

bollu commented Jul 14, 2015

@campagnola is AGG ported? the AGG example seems to be working for me right now. Or is it using some other anti-aliasing technique?

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 14, 2015

Member

I think if the example works, it must be ported. You might be able to tell from the diff, I'll look...

Member

larsoner commented Jul 14, 2015

I think if the example works, it must be ported. You might be able to tell from the diff, I'll look...

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 14, 2015

Member

Yeah, looking at the massive diff you can see appropriate changes to _AggLineVisual, and see commit history here there is a comment that AGG works:

#928 (commits)

So I guess we can move on. If you feel like you've debugged the lighting thing as much as you can, @bollu we should have a chat about next steps soon. I think @campagnola is planning to merge this as soon as he has time to work out remaining bugs.

Member

larsoner commented Jul 14, 2015

Yeah, looking at the massive diff you can see appropriate changes to _AggLineVisual, and see commit history here there is a comment that AGG works:

#928 (commits)

So I guess we can move on. If you feel like you've debugged the lighting thing as much as you can, @bollu we should have a chat about next steps soon. I think @campagnola is planning to merge this as soon as he has time to work out remaining bugs.

@bollu

This comment has been minimized.

Show comment
Hide comment
@bollu

bollu Jul 15, 2015

Contributor

I'll fiddle with the lighting, but I don't think it should be blocking for
the pull request to get merged.

As for the plotting, can we open up the old issue about the plotting API so
it can be discussed in greater detail? (Like, with more context, now that
the scene graph changes will be merged)

On Wed, Jul 15, 2015, 05:25 Eric Larson notifications@github.com wrote:

Yeah, looking at the massive diff you can see appropriate changes to
_AggLineVisual, and see commit history here there is a comment that AGG
works:

#928 (commits)
#928 (commits)

So I guess we can move on. If you feel like you've debugged the lighting
thing as much as you can, @bollu https://github.com/bollu we should
have a chat about next steps soon. I think @campagnola
https://github.com/campagnola is planning to merge this as soon as he
has time to work out remaining bugs.


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

Contributor

bollu commented Jul 15, 2015

I'll fiddle with the lighting, but I don't think it should be blocking for
the pull request to get merged.

As for the plotting, can we open up the old issue about the plotting API so
it can be discussed in greater detail? (Like, with more context, now that
the scene graph changes will be merged)

On Wed, Jul 15, 2015, 05:25 Eric Larson notifications@github.com wrote:

Yeah, looking at the massive diff you can see appropriate changes to
_AggLineVisual, and see commit history here there is a comment that AGG
works:

#928 (commits)
#928 (commits)

So I guess we can move on. If you feel like you've debugged the lighting
thing as much as you can, @bollu https://github.com/bollu we should
have a chat about next steps soon. I think @campagnola
https://github.com/campagnola is planning to merge this as soon as he
has time to work out remaining bugs.


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

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 15, 2015

Member
Member

larsoner commented Jul 15, 2015

@campagnola

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jul 15, 2015

Member

Plot API already has axes in #928 since I needed that for my demos. I wanted to introduce it in a different PR, but there were too many bugfixes mixed in as well. @Eric89GXL you might want to review the changes to PlotWidget.

Member

campagnola commented Jul 15, 2015

Plot API already has axes in #928 since I needed that for my demos. I wanted to introduce it in a different PR, but there were too many bugfixes mixed in as well. @Eric89GXL you might want to review the changes to PlotWidget.

campagnola added some commits Jul 15, 2015

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 15, 2015

Member
Member

larsoner commented Jul 15, 2015

grid = vp.visuals.GridLines(color=(0, 0, 0, 0.5))
grid.set_gl_state('translucent')
fig[0, 0].view.add(grid)

This comment has been minimized.

@larsoner

larsoner Jul 15, 2015

Member

Eventually I think we should simplify these processes, so users can add a grid in one line, an xlabel in one line, etc. But we'll leave that for a future PR :)

@larsoner

larsoner Jul 15, 2015

Member

Eventually I think we should simplify these processes, so users can add a grid in one line, an xlabel in one line, etc. But we'll leave that for a future PR :)

fig[0:4, 0:4].plot(data, width=0, face_color=color + (0.05,), edge_color=None,
marker_size=10.)
fig[0:4, 0:4].plot(data, symbol='o', width=0, face_color=color + (0.02,),
edge_color=None, marker_size=4)

This comment has been minimized.

@larsoner

larsoner Jul 15, 2015

Member

And I think we want to be able to link the axes of the 2D plot to the histogram axes, but again we'll save that for later

@larsoner

larsoner Jul 15, 2015

Member

And I think we want to be able to link the axes of the 2D plot to the histogram axes, but again we'll save that for later

__all__ = ['Fig', 'PlotWidget']
from .fig import Fig # noqa
from .plotwidget import PlotWidget # noqa
from ..scene import * # noqa

This comment has been minimized.

@larsoner

larsoner Jul 15, 2015

Member

I'm not sure about this one -- do we need to populate this namespace with everything from scene?

@larsoner

larsoner Jul 15, 2015

Member

I'm not sure about this one -- do we need to populate this namespace with everything from scene?

@@ -30,8 +30,6 @@
"""
__all__ = ['SceneCanvas', 'Node']

This comment has been minimized.

@larsoner

larsoner Jul 15, 2015

Member

Eventually we will probably want to re-populate this so our docs aren't full of junk from all those import * calls below...

@larsoner

larsoner Jul 15, 2015

Member

Eventually we will probably want to re-populate this so our docs aren't full of junk from all those import * calls below...

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 15, 2015

Member

I looked through the code, and it LGTM. But realistically it would probably take days to fully review this many changes, so we might just have to trust that the iterations we did to get things working, combined with the tests and examples looking right, are sufficient here. @almarklein do you have time to take a look?

I'm working on a small PR now to fix a couple of small bugs, and hopefully get make examples closer to working.

Member

larsoner commented Jul 15, 2015

I looked through the code, and it LGTM. But realistically it would probably take days to fully review this many changes, so we might just have to trust that the iterations we did to get things working, combined with the tests and examples looking right, are sufficient here. @almarklein do you have time to take a look?

I'm working on a small PR now to fix a couple of small bugs, and hopefully get make examples closer to working.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 15, 2015

it seems like this was probably a pain to track down, kudos

it seems like this was probably a pain to track down, kudos

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jul 16, 2015

Owner

Not so bad, once I realized that the SDF rendering was working as expected.

Owner

campagnola replied Jul 16, 2015

Not so bad, once I realized that the SDF rendering was working as expected.

@bollu

This comment has been minimized.

Show comment
Hide comment
@bollu

bollu Jul 15, 2015

Contributor

@campagnola, @Eric89GXL so, Colorbar into the plotting API next, then?

And, sure. Here is the link to the Google Doc for discussion

Contributor

bollu commented Jul 15, 2015

@campagnola, @Eric89GXL so, Colorbar into the plotting API next, then?

And, sure. Here is the link to the Google Doc for discussion

@bollu

This comment has been minimized.

Show comment
Hide comment
@bollu

bollu Jul 15, 2015

Contributor

The pull request for the new colorbar in vispy.plot, will it be against vispy/vispy or campagnola/vispy?

I'm assuming the former. The diff will look ginormous though, until this branch is merged

Contributor

bollu commented Jul 15, 2015

The pull request for the new colorbar in vispy.plot, will it be against vispy/vispy or campagnola/vispy?

I'm assuming the former. The diff will look ginormous though, until this branch is merged

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 16, 2015

Member
Member

larsoner commented Jul 16, 2015

@campagnola

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jul 16, 2015

Member

FYI: I am travelling this week (again) so my time is limited. The volume visual is a hangup at the moment because it emits warnings about unused shader variables and I haven't found a quick fix for it yet.

Member

campagnola commented Jul 16, 2015

FYI: I am travelling this week (again) so my time is limited. The volume visual is a hangup at the moment because it emits warnings about unused shader variables and I haven't found a quick fix for it yet.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 16, 2015

Member
Member

larsoner commented Jul 16, 2015

@campagnola

This comment has been minimized.

Show comment
Hide comment
@campagnola

campagnola Jul 16, 2015

Member

It's because the filters are being attached in the wrong place--to the end of the calculateColor() function rather than main(). I tried separating the two functions but this is difficult because calculateColor() depends on some globals.

Member

campagnola commented Jul 16, 2015

It's because the filters are being attached in the wrong place--to the end of the calculateColor() function rather than main(). I tried separating the two functions but this is difficult because calculateColor() depends on some globals.

@larsoner

This comment has been minimized.

Show comment
Hide comment
@larsoner

larsoner Jul 27, 2015

Member

Closing for #1021 in case other PRs into the branch need to be done

Member

larsoner commented Jul 27, 2015

Closing for #1021 in case other PRs into the branch need to be done

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