alelievr / NodeGraphProcessor

Node graph editor framework focused on data processing using Unity UIElements and C# 4.6
https://github.com/alelievr/NodeGraphProcessor/projects/2
MIT License
2.27k stars 375 forks source link

Both an input node and an editable field #65

Closed andybak closed 4 years ago

andybak commented 4 years ago

I figured out that adding both Node and SerializeField attributes to a field give you an editable field plus a node input:

[Input("Amount"), SerializeField]

However this has a few issues.

  1. The editable field is not located next to the input creating a confusing UI
  2. VisibleIf only affects the editable field, not the node input.

I'd be happy to help out with this if you agree it's worth improving on.

andybak commented 4 years ago

On reflection - please ignore point 2. I'm not sure hiding an input node is the right thing to do. I should add a separate issue for that anyway.

alelievr commented 4 years ago

Hello,

This is an interesting idea :) I wonder how can we do something good in terms of UI for this though, it would add a lot of width to the node I think. If you have mock-ups or design proposition for the UI/UX I'd be glad to take a look :)

Side note: In another project, I have made something that hides the input field when it's port is connected (because the value is overwritten by the connected edge). Maybe we could do something similar here?

andybak commented 4 years ago

If you have mock-ups or design proposition for the UI/UX I'd be glad to take a look :)

I think it would be best to copy either VFX Graph or Shader Graph. I personally prefer VFX Graph but I'd be happy with either approach.

shadergraph vfxgraph

Maybe we could do something similar here?

That would be a partial solution. But the main UX problem is having the editable fields and the nodes in different places. I think a user would expect them to be the same "thing" irrespective or not of whether the node is connected.

alelievr commented 4 years ago

I like more the VFX graph approach, I think with ShaderGraph drawers you lose a lot of space.

But the main UX problem is having the editable fields and the nodes in different places

I'm not sure to understand if we choose the VFX graph approach, the field inputs are directly next to the port and won't be displayed in the node body.

andybak commented 4 years ago

I'm not sure to understand

Don't worry - I was just saying "the important thing is this" - and as you point out either solution will address this point.

alelievr commented 4 years ago

Ah okay :)

TeorikDeli commented 4 years ago

Well, I might help with this when I find time, if you want. We did implement some custom behaviour for Serialized Fields. I am attaching the screenshot; so if you like, I can add this.

Screen Shot 2020-08-27 at 17 49 05

Screen Shot 2020-08-27 at 17 53 46

andybak commented 4 years ago

That looks really nice.

TeorikDeli commented 4 years ago

OK then, I will add this; but it could take some time 😁

alelievr commented 4 years ago

Yep, I agree this is a very nice implementation 🙂 I'd glad to help if you want

TeorikDeli commented 4 years ago

85 Well, I hope you like it 😅

alelievr commented 4 years ago

:shipit:

I guess I'll close this issue now 🙂 Let me know if you find any issue with this new drawer system

And thanks again @TeorikDeli 😄

alelievr commented 4 years ago

side-note: I updated the readme: https://github.com/alelievr/NodeGraphProcessor/blob/master/README.md#field-drawers-thanks-teorikdeli

liuxinjia commented 4 years ago

There is something wrong with list or array. image After Disconnect or connect again: image.

liuxinjia commented 4 years ago

It happens when the port is belong to a list field or array field. image

TeorikDeli commented 4 years ago

Hmmm, I think we should exclude Lists or Arrays (because it is hard to show that in such a small space). By the way, is there any error on the console?

liuxinjia commented 4 years ago

No error on the console but the result is weird. We use array to generate multiple ports with the help of CustomPortbehaviourAttribute

alelievr commented 4 years ago

I agree to exclude lists/arrays type from having a drawer. I'll exclude every type that implements IEnumerable

andybak commented 4 years ago

I'll exclude every type that implements IEnumerable

Hmmmm. Trying to think how to handle an input that can take a single value or a list. I've been using the auto conversion to handle this.

Ideally it would draw it as a float input but also accept an IEnumerable. Any ideas how to do this easily?

(Most of my inputs are like this. It's a powerful tool and I got the idea from a few other node-based systems)

alelievr commented 4 years ago

I'm not sure to understand, are you talking about instead of disabling the drawer on collections, we can show it but only for the first element of the collection?

andybak commented 4 years ago

Almost. I'm thinking about inputs designed to be polymorphic.

So for example an input can accept a float or a list of floats. (I have a conversion method that wraps a single float in a 1 item list).

The common case is a single float and this is what I would want the drawer to support. However if I define the input as a List then I don't get the drawer.

I think the ideal solution would be to optionally override the type that is drawn,

So I could maybe do something like

[ShowAsDrawer(float), Input("Value")] public List<float> val;

alelievr commented 4 years ago

I see, small question though: these drawers directly sets the value into the field using reflection, which will break in this case. Also I don't think we can use the CustomPortInput/Output because there is no edge connected to the port and it wouldn't make sense. So I guess the only solution would be to use the convertions defined in TypeAdapter?

andybak commented 4 years ago

I'll take your word for it on the above as I still haven't had a chance to become familiar with any of the internals.

I think going via convertions (shouldn't that be "conversions"?) makes sense for my case as that's how I'm handling polymorphism.

Let me know if I can provide any more explanation that would help. I get the sense this is something that would be generally useful rather than something peculiar about my intended application but I might be mistaken on this.

alelievr commented 3 years ago

shouldn't that be "conversions"?

Yeah I need to fix this typo ^^'

Note that I pushed a change that prevents IEnumerable fields to have a drawer: eed2046f2a610a788a083c0fc47d9ac4e00a0549 But that's temporary, I'll probably add something like you suggested [ShowAsDrawer(Type)] that will handle convertion through a TypeAdapter.