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.

Sign up for GitHub

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

Jump to bottom

Visual Script Fixes #37053

Closed
wants to merge 1 commit into from
Closed

Visual Script Fixes #37053

wants to merge 1 commit into from

Conversation

jdbener
Copy link
Contributor

@jdbener jdbener commented Mar 14, 2020 •
edited
Loading

  1. Fixed bug causing variable setters/getters added from the add node menu to be unusable when only one variable is present.
  2. Updated setter/gettter port naming to make the node's state more clear.
  3. Fixed regression causing script members to not update.

@jdbener jdbener force-pushed the VisuaScriptFixes branch 2 times, most recently from 0817905 to cb7d287 Compare March 15, 2020 00:17
@Chaosus Chaosus added this to the 4.0 milestone Mar 15, 2020
@swarnimarun
Copy link
Contributor

Nice.
But is there a reason for you to have rename _update_members ? I had fixed it on my branch(I am waiting for more breaking changes to get committed before making PRs) and there wasn't a need for the change, something I missed?
Secondly, I would recommend to keep all the method binds inside of VisualScriptEditor::_bind_methods() for easier maintainability.

And at last I am not sure why it was required to add another function to the VisualScriptNode class.?

swarnimarun
swarnimarun reviewed Mar 16, 2020
View reviewed changes
modules/visual_script/register_types.cpp Outdated
@@ -116,6 +116,10 @@ void register_visual_script_types() {
#ifdef TOOLS_ENABLED
ClassDB::set_current_api(ClassDB::API_EDITOR);
ClassDB::register_class<_VisualScriptEditor>();

ClassDB::register_class<VisualScriptEditor>();
ClassDB::bind_method(D_METHOD("update_members"), &VisualScriptEditor::update_members);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move it inside VisualScriptEditor::_bind_methods()

@swarnimarun swarnimarun mentioned this pull request Sep 17, 2022
Open
5 tasks
@jdbener
Copy link
Contributor Author

jdbener commented Mar 16, 2020 •
edited
Loading

And at last I am not sure why it was required to add another function to the VisualScriptNode class.?

There wasn't a virtual function which got executed when a node was added to the graph. I could have made validate_input_default_values() fit the bill but that seamed obscure. Seam reasonable?

@jdbener
Visual Script Fixes
2354c4a 
1) Fixed bug causing variable setters/getters added from the add node menu to be unusable when only one variable is present.
2) Updated port naming to make the node's state more clear.
3) Fixed regression causing script members to not update.
@swarnimarun
Copy link
Contributor

@fire, @akien-mga I think we can merge it if everyone is fine with adding a new function to VisualScriptNode class.

@swarnimarun
Copy link
Contributor

I believe,
We could just check which is the first property in the Constructor as well considering VisualScript has to be created before Nodes.
@jdbener would it not have worked?

Otherwise it's fine by me.

@jdbener
Copy link
Contributor Author

jdbener commented Apr 3, 2020 •
edited
Loading

We could just check which is the first property in the Constructor as well considering VisualScript has to be created before Nodes.

@swarnimarun That was my first solution, changing the property in the constructor, and I saw the property was getting assigned whenever you opened the add node menu but not when it was placed in the graph. I then tried every single virtual function to try and find one which might be obscurely named but called as a node entered the graph. I was also considering using validate_input_default_values() but it isn't a virtual function. If you prefer making validate_input_default_values virtual would be my next solution.

@swarnimarun
Copy link
Contributor

Hmmm. That's something I didn't expect but if that's the case then I believe it's fine to add another function in VisualScriptNode.

@Gallilus Gallilus mentioned this pull request Jul 13, 2021
Merged
@akien-mga
Copy link
Member

This was superseded by #49749, but it would be worth checking if all fixes from this PR are fully superseded or if some parts of it should be put in a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swarnimarun swarnimarun swarnimarun left review comments

Assignees
No one assigned
Labels
archived bug topic:editor topic:visualscript
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jdbener @swarnimarun @akien-mga @Calinou @Chaosus