-
-
Notifications
You must be signed in to change notification settings - Fork 454
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
Remove type from NodeInput::Network #2079
Conversation
fc21991
to
45fd246
Compare
The Into node would be inserted before the flattening step. For example instead of having the To Group and To Element nodes in the merge node, they would be inserted automatically. This is done by inserting an Into node of the type defined for that import. The type system for the parameter passed when evaluating the top level network is unrelated to the Into node insertion, and not something I am too familiar with. |
@adamgerhant your proposal is not insert a ToType node before the inputs (e.g. ToGraphicGroup). In order to determine the type required (here GraphicGroup) is it not necessary to resolve the types? |
No, the ToType node does not depend on any compiled types, it is simply what the user or definition sets it to be |
@adamgerhant then if the user wishes to change the functionality of their custom node, they need to update the graph and then explicitly change the input type. Why not just do this automatically? It seems like your approach would make the UX worse. |
The IntoNode would not be displayed in the graph. As a default, they could set the input type to Generic(T), which inserts an IntoNode that acts as an identity node. |
@0HyperCube @adamgerhant I don't know the status of this but would the two of you be able to work it out together? Maybe on Discord so I can read your discussion and jump in if relevant. |
This PR is not related to automatic Into node insertion. @TrueDoctor should probably verify this one to ensure it is correct. |
Partially addresses #2042 (this removes the type from NodeNetwork::Input).
In the issue it mentions that it is necessary to store input types in the
NodeNetwork
struct for the into system. I am unsure of how exactly the into insertion system would be implemented. However I don't believe the proposed change would help at all.In order to insert an into node, you need to resolve the type of the input. Types are only resolved after flattening. After flattening the nested network inputs are no longer accessible. This makes storing a list of input types in the
NodeNetwork
entirely redundant.This change necessitated passing the types to the top level network to the compile function. This is only required for the inputs to the TOP LEVEL node network. This doesn't impact copy pasting or in any other way limit the flexibility of the graph. When you compile a network and then call it, you already know what types you are going to call it with (for the editor this is
concrete!(RenderConfig)
).Even if further work is required for this issue, I think this PR represents an improvement as it avoids storing redundant and unused data within the complex network data structures.