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

Remove type from NodeInput::Network #2079

Closed
wants to merge 1 commit into from

Conversation

0HyperCube
Copy link
Member

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.

@0HyperCube 0HyperCube changed the title deRemove type from NodeInput::Network Remove type from NodeInput::Network Oct 26, 2024
@adamgerhant
Copy link
Collaborator

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.

@0HyperCube
Copy link
Member Author

@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?

@adamgerhant
Copy link
Collaborator

No, the ToType node does not depend on any compiled types, it is simply what the user or definition sets it to be

@0HyperCube
Copy link
Member Author

@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.

@adamgerhant
Copy link
Collaborator

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.

@Keavon
Copy link
Member

Keavon commented Nov 11, 2024

@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.

@adamgerhant
Copy link
Collaborator

This PR is not related to automatic Into node insertion. @TrueDoctor should probably verify this one to ensure it is correct.

@0HyperCube 0HyperCube closed this Nov 16, 2024
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.

3 participants