Skip to content

Added search node functionality#40

Merged
HungryProton merged 1 commit intoprotongraph:developfrom
christinoleo:develop
May 17, 2020
Merged

Added search node functionality#40
HungryProton merged 1 commit intoprotongraph:developfrom
christinoleo:develop

Conversation

@christinoleo
Copy link
Contributor

I was missing the search functionality from the search bar of the "add new node dialog". It is not the most efficient implementation, but I believe it works well. Let me know if you prefer some other way.

@christinoleo
Copy link
Contributor Author

I also just noticed it fixes issue #2

@HungryProton
Copy link
Member

HungryProton commented May 17, 2020

Thanks for the PR
Don't worry about making one PR for each separate features. Searching nodes works really well but enabling multi-input on a single slot causes more issues than it solves because of how the concept node collects its inputs and how multi-threading is setup. (Multi-threading is disabled for now though)

I'll try to merge the code from parent_to_node in the base class as soon as I can

@christinoleo
Copy link
Contributor Author

christinoleo commented May 17, 2020

Ah, good to know about the multi-threading. I didn't yet try to understand that part. I guess I'll change to dynamically add a new input when the previous input is populated, though I am not sure how godot will handle that.

I will also keep in mind that to send PRs with more changes at once is better for you.
Thank you for the feedback!

Regarding the resource saver from output node (#41 ) , I added a new String input and use that as the path for a ResourceSaver call. I believe it should actually be the first input (not the second), but then it would break all other Output nodes. What do you think?

Sidenote: I just saw your video in GDQuest. Congrats!

@HungryProton
Copy link
Member

HungryProton commented May 17, 2020

Sorry I didn't wrote my sentence properly, I meant to say it's totally fine to make one PR for each feature, it makes it easier this way.

Regarding the output node, I think the best way be to not change the ouput node at all, but create a completely new node called ResourceSaver (or anything else, I'm not good at naming things). You can leave it in the Output category I think.

This new node would take the node input and the string input in your preferred order and save the scene from there. This way it doesn't break backward compatibility and it doesn't force the feature to be always on, people can just add the node if they need it.

@christinoleo
Copy link
Contributor Author

christinoleo commented May 17, 2020

Ah, ok! I cleaned up this the PR. Sorry for merging all here btw, it was actually by mistake.
I will some other ones regarding the other changes.

Good idea on the new node for ResourceSaver! Will do that.

@christinoleo christinoleo force-pushed the develop branch 2 times, most recently from 6725ffb to a6f53c8 Compare May 17, 2020 17:20
@HungryProton
Copy link
Member

Thanks! Merging the search node feature

@HungryProton HungryProton merged commit 94c97d1 into protongraph:develop May 17, 2020
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.

2 participants