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

Node layout links structure #562

Closed
tibicen opened this issue Jul 26, 2019 · 12 comments
Closed

Node layout links structure #562

tibicen opened this issue Jul 26, 2019 · 12 comments
Labels
do not close issue that should stay open (avoid automatically close because stale) feature request feature request from the community

Comments

@tibicen
Copy link

tibicen commented Jul 26, 2019

Hi I've encountered some unusuall default node connections.

By default Nodes are often connected Input > Input.
My proposal is to connect previous node Output > Input.
This is rather unusual, but of course it is nice to have that option, not making it more constrained. The problem is, the default option is hardly readable, and confusing. And of course connecting output from "previous" node is the would end in the same results:
obraz
It is not only about only showed nodes, but principle.
What do you think about this small change?

@natowi
Copy link
Member

natowi commented Jul 26, 2019

I have thought about this, too (#269).

default-node-graph-color1original

default-node-graph-color

@tibicen
Copy link
Author

tibicen commented Jul 26, 2019

I found the place responsible for this, and would like to work on it to fix this, if there are no one against it, anyone can give me a green light?

@natowi
Copy link
Member

natowi commented Jul 26, 2019

@yann-lty Are there any reasons for the current node layout?

@yann-lty
Copy link
Member

I get why this can be confusing, but the idea was to keep a clear layout when starting to multiply variants. If we push your idea, we would have a link from CameraInit.output to all nodes until StructureFromMotion. And then StructureFromMotion.output to all nodes until the end of the pipeline.
If we stay in a simple pipeline, I guess this is okay. But I think this might make things harder to read in more complex use-cases:
Input > Input (current implementation)
image
Output > Input (proposed implementation)
image

But I totally agree on the fact that the graph editor would greatly benefit from UI improvements to make it more readable and better appreciate connections between nodes on a visual level.

@natowi
Copy link
Member

natowi commented Jul 26, 2019

Thank you for the explanation. How about an option to switch between both variants?
An alternative could be (as proposed in #269 -10) "when passing data through a node (A->B->C) do not (by default) connect C to input of B, (connect to output of A or provide better connector routing) or create a pass-through output for B."

@tibicen If you can come up with a good solution, this would be a useful contribution

@tibicen
Copy link
Author

tibicen commented Jul 26, 2019

Thank you for the explanation. How about an option to switch between both variants?

I think that could leave some confusion. In my opinion default graph, should be consistent, and straight without any additional steps (like popus etc). I'm kind of guy: "Less buttons, less options -> more functionality".

I would stick with one option.

@yann-lty It is right that first example looks clean, but it confues how the nodes works. It just hides connection under the nodes. Second picture (however noodly it looks) shows real realtions, between nodes/folders/relations.

  1. "Pass-through output" - that is just additional element that only func is to pass element through, that unnecesarry complication, and more options.
  2. Current state - looks clean for noob/advanced user, but doesn't really show real relations.
  3. Both variants - i would stick to one default option
  4. After some thought I would still recommend "noodly" sollution with proper connection. It might look messy, but it could really help for newcomers to understand nodes relations.

P.S. I would also consider renaming "input and output for proper names. But I don't know if I should make separate issue for that?

@MightyBOBcnc
Copy link

MightyBOBcnc commented Jul 26, 2019

The current implementation is visually clean, but it is very confusing and doesn't conform to the standard in other DCC programs. I think the messiness (https://user-images.githubusercontent.com/1674646/61945652-d124f880-afa0-11e9-875b-9d3f09c389fe.png) would be preferable if it eliminates the confusion of inputs connecting to other inputs. Messiness is just a fact of life in a complex node system. Unreal Engine 4 example: https://i.imgur.com/QFV3D74.jpg

One thing that could be introduced to clean up messy node graphs would be reroute "nodes" like in UE4 (double click on a spline) or Substance Designer (hold down Shift+Alt to toggle visibility). These are simply control points that the user can insert to manually control the spline flow. You can actually see a few of these points in the UE4 example above (points controlling the spline flow in the bottom left). Here's a Substance Designer example: https://www.youtube.com/watch?v=gihP4nFiaLw

An alternative that might possibly be both clean and intuitive (but would require more work to implement) could be to enforce a grid in the node graph and instead of allowing the splines to be "noodles" force the splines connecting the nodes also follow the grid like in flow chart software (this example created with www.draw.io): https://i.imgur.com/5s0LudX.jpg

@tibicen
Copy link
Author

tibicen commented Jul 27, 2019

Agree with @MightyBOBcnc, except constraint it to grid layout.
I like to think that there are different people with different approaches. Wheather they are thinking linear, tree-like or even in circles. I would leave node layout flexibility, with a cost of messines.

I could try implement simple reroute node that handles one link in and many links out. As it is said I would recommend cleaning the inputs and outputs in groups, like:
{ImageMatching.inputs.featuresFolder}
{ImageMatching.outputs.output}
{StructureFromMotion.outputs.extraInfoFolder}
and rename all input and output sockets into proper names. Of course leaving the possibility to connect input to input.
Additionally create func to update all the old .mg files to new ones.

@natowi
Copy link
Member

natowi commented Sep 18, 2019

Node redesign #612

@natowi natowi added feature request feature request from the community do not close issue that should stay open (avoid automatically close because stale) and removed type:enhancement labels Oct 27, 2019
@stale
Copy link

stale bot commented Feb 24, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale for issues that becomes stale (no solution) label Feb 24, 2020
@natowi natowi removed the stale for issues that becomes stale (no solution) label Feb 24, 2020
@natowi
Copy link
Member

natowi commented Jan 10, 2021

This is a preview of the upcoming node design:

mr

(pass-through node connections are dotted lines)

And it will be possible to create dependent node settings

mr2

@natowi natowi added this to the Meshroom 2021.1.0 milestone Jun 27, 2021
@natowi
Copy link
Member

natowi commented Jun 27, 2021

Closed by #612

@natowi natowi closed this as completed Jun 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not close issue that should stay open (avoid automatically close because stale) feature request feature request from the community
Projects
None yet
Development

No branches or pull requests

4 participants