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

Shift paradigm from layers to nodes #254

Merged
merged 1 commit into from Aug 20, 2016
Merged

Shift paradigm from layers to nodes #254

merged 1 commit into from Aug 20, 2016

Conversation

sigvef
Copy link
Member

@sigvef sigvef commented Jul 20, 2016

image

This commit shifts the central paradigm of nin from being a layer-based
system to being a node-based system. Not all features have been ported
over to the new system, although it is certainly complete enough to be
used to make demos. Hopefully we are willing to merge this sooner rather
than later, and we can then later fix the rest of the features as they
are needed.

  • Overview

Before, a demo in nin would consist of several layers in an ordered
list. Each layer would have a start time and end time, and each
layer's update and render methods would be called with the usual
update/render call semantics while demo time was within a layer's start
and end times. Each layer exposed an EffectComposer pass, and all passes
for active layers were chained together in a top-to-bottom such that the
next layer could optionally access the rendered data of the previous
layer.

Now, a demo in nin consists of several nodes in a graph. Nodes do not
have start times or end times. Rather, the graph is traversed from a
designated root node in order call update and render for each of the
reachable nodes. A node may enable or disable their inputs, which serves
as temporary graph modification in order to "connect and disconnect"
different nodes at different times in the demo. This is the spiritual
replacement of start times and end times from the layer system. The big
advantage of the new node-based system over the old layer system is
that it is now easier to create more advanced multi-pass rendering
pipelines.

  • Nodes and the graph

Nodes have different types, and all nodes inherit from the base
NIN.Node. A node type may declare a number of named and typed inputs and
outputs. Nodes may produce data and store it in their outputs. Connected
nodes may then read that same data from their inputs. The directed graph
of connections from node outputs to node inputs may not contain cycles.

The graph is stored in res/graph.json, which is analogous to the old
res/layers.json. Because of the frequency of graph edits in the UI
(particularly dragging nodes to change their position), and in the
interest of simplicity and performance, res/graph.json is not actively
watched for changes on disk after the first load at the beginning of
nin run. The intention is that most changes to res/graph.json should
be made via the UI.

Currently, all graphs must have a node of type NIN.RootNode called
root in order for the demo to update and render properly. This node is
the root node that is used for traversal during update and render, and
the texture made available to the root node's only input root.screen is
what is finally showed on the screen.

The LayerManager of old has been replaced with an analogous NodeManager.

  • Current state of features

This section elaborates upon the different features of nin and their
current state with regards to the paradigm shift. Features that are not
affected are probably not mentioned here.

** Node graph viewer / editor

Replacing the old layer viewer/editor, the node graph viewer / editor
currently supports visualization of the graph including connections and
activeness of nodes. Nodes are persistently draggable. The viewer is
pan/zoom-enabled, but has no pan/zoom persistence. It is currently not
possible to create or delete nodes or connections.

** Generate

This has not been touched, so it most likely does not work.

** Compilation

This has not been touched, so it most likely does not work.

** Init

This has not been touched, so it most likely does not work.

** Backend res/layers.json / res/graph.js manipulation

This has been updated and works for whatever it is being used for in
this commit, but has not been scrutinized for completeness.

** Shader updates

This is probably broken. Perhaps we can leverage the node system in
order to make it less grep-y in the future.

** Camera paths, fly-around mode

This is probably broken.

** Layer / Node inspection

Nodes can no longer be inspected, but inputs and outputs can. Currently,
the only inspectable inputs/outputs are texture input/outputs. When
inspected, they will render their texture to the demo screen after the
regular render pass is finished.

@sigvef
Copy link
Member Author

sigvef commented Jul 20, 2016

See https://github.com/sigvef/nin-node-test-demo for a project that works with this prq.

@iver56
Copy link
Member

iver56 commented Jul 20, 2016

How about backwards compatibility with existing demos? Should we simply create a release before this prq is merged, and use that release for old demos?

@sigvef
Copy link
Member Author

sigvef commented Jul 20, 2016

This will not work with any old demos at all. Just like most other changes to nin ;)

@aleksanb
Copy link
Member

Currently each layer is wrapped by a closure, and the layer is stored on window such as:

global.SceneSwitcherNode = SceneSwitcherNode;
})(this);

If we use es6 modules or something similar, or wrap the nodes outside the file, we can reduce the amount of boilerplate!

@aleksanb
Copy link
Member

Yeah, let's tag current head with LEGACY and merge this into master when it has been reviewed.

@iver56
Copy link
Member

iver56 commented Jul 20, 2016

@sigvef
Copy link
Member Author

sigvef commented Jul 20, 2016

If we use es6 modules or something similar, or wrap the nodes outside the file, we can reduce the amount of boilerplate!

I agree, that would be nice. From what I'm reading, using proper es6 modules require a smarter packer/assembler than the current concat-and-maybe-optimize scheme we're using right now. Do you have a concrete example of how it could be done for us without resorting to changing the build flow? I'm incidentally also concerned about 'use strict' used without closures.

One way we could do this is is something like:

(function(NIN) {
  NIN.SomethingNode = class extends NIN.Node {
    ..
  }
})(this.NIN);

I'm not sure if this is semantically any different from what we have now, so it might just come down to a matter of preference.

Changing the build flow down the line might be useful, but I'm not sure it should be a priority for now.

}
return;
}
var index;
Copy link
Member

Choose a reason for hiding this comment

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

Equivalent to var index = array.find(node => node.id == nodeId)

Copy link
Member Author

Choose a reason for hiding this comment

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

array.findIndex(..), but otherwise nice catch!

@aleksanb
Copy link
Member

I couldn't find relativeFrame / frame in the nodes.
Seems like nodes only receive the global frame counter now?
This would make animating scenes with our current workflow rather tricky,
having to handle subtraction of the nodes startFrame within the node itsel?

@cristeahub
Copy link
Member

I too would like to do a full review of this before this is merged. Such a major change will benefit from multiple people looking at it. We can halt creating new features for nin to make this change easier.

@stianjensen
Copy link
Member

The layer zooming seems to be broken.
Zooming in works, but zooming out does not.

From my testing, it seems to be caused the fact that the horizontal scrollbar is now caused by overflow on the body element, and the calculations for xScale does not work anymore.
Setting the .bottom element to width:100%;overflow-x:scroll and hardcoding the width of .layers-bar-container to the same width as the waveform fixes this issue for me.

This also has the effect that when scrolling in the bottom timeline, the whole body element is not scrolled.

@stianjensen
Copy link
Member

What are your thoughts on how demos should be structured into scenes or separate parts with this new system?

In the demo project, there is a SceneSwitcherNode. Would we create one big SceneSwitcherNode for each demo which deals with enabling and disabling different nodes, or would nin provide some sort of builtin, more declarative, system for dealing with this?

@sigvef
Copy link
Member Author

sigvef commented Jul 31, 2016

On time/scene structure in demos: I opted for a hands-off approach in nin that leaves scene switching largely in the hands of the demo developer. Nin supports enabling and disabling of node inputs in order to effectively connect and disconnect subtrees from the graph, but does not lay down any conventions on how and when these node inputs should be enabled or disabled. One way of handling scene switching would be to have a multiplexing node with hard-coded timestamps (like SceneSwitcherNode). Another would be to have a multiplexer that triggers scene switches by other exotic inputs such as midi, aunino, or other things. Ultimately, I expect a convention for scene switching will emerge across future demos made with nin. At that point we can consider bringing a more opinionated scene switcher into nin.

@sigvef
Copy link
Member Author

sigvef commented Jul 31, 2016

On horizontal scrolling in time: perhaps we don't even need horizontal zoom any more? Perhaps a fully zoomed-out full-width view is enough.

@stianjensen
Copy link
Member

On horizontal scrolling in time: perhaps we don't even need horizontal zoom any more? Perhaps a fully zoomed-out full-width view is enough.

That's true. It's probably better to just remove it.

@aleksanb
Copy link
Member

I think we should be pragmatic and make a default implementation when it comes to scene switching.
It's such a core feature that it'll be a shame if everyone has to roll their own.
If everyone has to make their own sceneswitcher node we're going to end up with a fragmented mess and higher threshold to make new projects .

@cristeahub
Copy link
Member

I think we should be pragmatic and make a default implementation when it comes to scene switching.
It's such a core feature that it'll be a shame if everyone has to roll their own.

I agree here. At least one way to do it now with good support as a starting point. Should some better feature emerge, then we can always change the default scene switcher, or have multiple.

@sigvef
Copy link
Member Author

sigvef commented Jul 31, 2016

Ok, how do you propose the default scene switcher should work? As a multiplexer node presumably, but with how many inputs? How should switch timestamps be specified? What about transitions between scenes, e.g. cross-fades, wipes, etc work (node-based nin finally makes these things easy to implement)? What about cutting back and forth between scenes? These are questions that we need to answer before we can make a default scene switcher. Answering them probably requires some testing and experimentation to see what works and what doesn't, which is why I'm suggesting that we defer adding a default scene switcher until we know more about how we would want a default scene switcher to work. That is, let's make some demos, and then use what we've learned from those demos to choose a default scene switcher for nin.

@sigvef
Copy link
Member Author

sigvef commented Jul 31, 2016

To be clear: I do want a default scene switcher, but I don't think we should add it right now.

@cristeahub
Copy link
Member

I've started the review here, and will continue tomorrow. Will hold off my feedback until tomorrow, as I think I might need to revisit some of my thoughts 👍

@cristeahub
Copy link
Member

cristeahub commented Aug 12, 2016

I like the change, it does seem like a way to do a lot of what we want easier, however I do have some thoughts:

  • The graphs will become very big and incomprehensible: A solution here could be to hide parts of the graph in a vitual supernode or something. I.e. one scene might consist of 20 nodes and they can collapse to 1 virtual node that just says scene 1, when clicked it can show the 20 nodes it's made up of.
  • We can't easily see how time affects the demo: We lose a sense of what happens where with this new system. I have no good idea here yet, and this might be fixed by our down-the-road scene system. As of right now it seems almost to me that we could have our old scene layout, but with each scene being its own graph. Would like input here!
  • This breaks a lot: Since this a big breaking change we will have to rewrite a lot of nin. This isn't an issue, but we just need to be fully aware of this. We can't expect to create a fully fledged demo with this system until a lot more is in place. We have the layer-based release (https://github.com/ninjadev/nin/releases/tag/v0.1.0) so we can always fall back on this.
  • We need some administrative work to change issues and so on: After this big change a lot of our issues and written material will be outdated. At least the issues should be revisited and cleaned up with this.

for (var i in sortedPaths) {
console.log('first we need one of these!', sortedPaths[i]);
Copy link
Member

Choose a reason for hiding this comment

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

This message seems a bit out of place. Maybe change the text or consider removing it. I guess it was for debug purposes?
image

@sigvef
Copy link
Member Author

sigvef commented Aug 15, 2016

ping

@sigvef
Copy link
Member Author

sigvef commented Aug 15, 2016

(I haven't fixed horizontal zooming, but perhaps we can defer that to a later pull request).

@stianjensen
Copy link
Member

I'm gonna go ahead and get this merged now, before the pull request gets even longer and harder to follow. @cristeahub brought up some nice points in his summary, but I think all of those points can be addressed in future changes.

Can you squash your commits Sigve, and then I'll get this into master?

@stianjensen stianjensen mentioned this pull request Aug 20, 2016
This commit shifts the central paradigm of nin from being a layer-based
system to being a node-based system. Not all features have been ported
over to the new system, although it is certainly complete enough to be
used to make demos. Hopefully we are willing to merge this sooner rather
than later, and we can then later fix the rest of the features as they
are needed.

* Overview

Before, a demo in nin would consist of several layers in an ordered
list. Each layer would have a start time and end time, and each
layer's update and render methods would be called with the usual
update/render call semantics while demo time was within a layer's start
and end times. Each layer exposed an EffectComposer pass, and all passes
for active layers were chained together in a top-to-bottom such that the
next layer could optionally access the rendered data of the previous
layer.

Now, a demo in nin consists of several nodes in a graph. Nodes do not
have start times or end times. Rather, the graph is traversed from a
designated root node in order call update and render for each of the
reachable nodes. A node may enable or disable their inputs, which serves
as temporary graph modification in order to "connect and disconnect"
different nodes at different times in the demo. This is the spiritual
replacement of start times and end times from the layer system.  The big
advantage of the new node-based system over the old layer system  is
that it is now easier to create more advanced multi-pass rendering
pipelines.

* Nodes and the graph

Nodes have different types, and all nodes inherit from the base
NIN.Node. A node type may declare a number of named and typed inputs and
outputs. Nodes may produce data and store it in their outputs. Connected
nodes may then read that same data from their inputs. The directed graph
of connections from node outputs to node inputs may not contain cycles.

The graph is stored in res/graph.json, which is analogous to the old
res/layers.json. Because of the frequency of graph edits in the UI
(particularly dragging nodes to change their position), and in the
interest of simplicity and performance, res/graph.json is not actively
watched for changes on disk after the first load at the beginning of
`nin run`. The intention is that most changes to res/graph.json should
be made via the UI.

Currently, all graphs *must* have a node of type NIN.RootNode called
root in order for the demo to update and render properly. This node is
the root node that is used for traversal during update and render, and
the texture made available to the root node's only input root.screen is
what is finally showed on the screen.

The LayerManager of old has been replaced with an analogous NodeManager.

* Current state of features

This section elaborates upon the different features of nin and their
current state with regards to the paradigm shift. Features that are not
affected are probably not mentioned here.

** Node graph viewer / editor

Replacing the old layer viewer/editor, the node graph viewer / editor
currently supports visualization of the graph including connections and
activeness of nodes. Nodes are persistently draggable. The viewer is
pan/zoom-enabled, but has no pan/zoom persistence. It is currently not
possible to create or delete nodes or connections.

** Generate

This has not been touched, so it most likely does not work.

** Compilation

This has not been touched, so it most likely does not work.

** Init

This has not been touched, so it most likely does not work.

** Backend res/layers.json / res/graph.js manipulation

This has been updated and works for whatever it is being used for in
this commit, but has not been scrutinized for completeness.

** Shader updates

This is probably broken. Perhaps we can leverage the node system in
order to make it less grep-y in the future.

** Camera paths, fly-around mode

This is probably broken.

** Layer / Node inspection

Nodes can no longer be inspected, but inputs and outputs can. Currently,
the only inspectable inputs/outputs are texture input/outputs. When
inspected, they will render their texture to the demo screen after the
regular render pass is finished.
@sigvef
Copy link
Member Author

sigvef commented Aug 20, 2016

ping

@stianjensen stianjensen merged commit 782a977 into master Aug 20, 2016
@stianjensen stianjensen deleted the paradigm-shift branch August 20, 2016 11:43
@stianjensen
Copy link
Member

🍫 🍰 🍸 🎉

@cristeahub
Copy link
Member

:cheer:

@stianjensen
Copy link
Member

:missing_emoji:

@aleksanb
Copy link
Member

aleksanb commented Nov 5, 2019 via email

@sigvef
Copy link
Member Author

sigvef commented Nov 5, 2019

Yes: 782a977...master

@aleksanb
Copy link
Member

aleksanb commented Nov 5, 2019

Great.

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

5 participants