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

Improve Visual Script editor to suggest the proper visual script nodes. #56252

Merged

Conversation

Gallilus
Copy link
Contributor

@Gallilus Gallilus commented Dec 26, 2021

proposal #3528

While filters are preset they are fully editable while searching.
The base is set relative to what input the editor gives.

  • It is way easier to use constants.
  • Its is possible to find content, not inside the base.

image
image

Property_selector.mp4

Fixes: godotengine/godot-proposals#3528

@Gallilus Gallilus requested a review from a team as a code owner December 26, 2021 15:31
@Gallilus Gallilus marked this pull request as draft December 26, 2021 15:31
@Calinou Calinou added this to the 4.0 milestone Dec 26, 2021
@Gallilus Gallilus force-pushed the Update-visual-script-property-selector branch 3 times, most recently from 49b6d51 to 6144566 Compare December 28, 2021 17:24
@Gallilus Gallilus marked this pull request as ready for review December 28, 2021 18:14
@Gallilus Gallilus force-pushed the Update-visual-script-property-selector branch from 6144566 to efa1bef Compare January 1, 2022 14:20
@Gallilus
Copy link
Contributor Author

Gallilus commented Jan 1, 2022

added a few tweaks and easier access to deconstructors.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Approved on the design.

Need to build and check for crashes and mistakes on a technical review.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Do the icons have tooltips?

@Gallilus
Copy link
Contributor Author

Gallilus commented Jan 1, 2022

Yes, even the searchbar

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@akien-mga
Copy link
Member

Neither the current PR title nor the commit message ("Update visual script autocomplete") seem to be an accurate description of what this PR does, could you amend them?

See https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-messages-with-readability-in-mind

@fire
Copy link
Member

fire commented Jan 4, 2022

@Gallilus What do you think about this phrasing?

Improve Visual Script editor to suggest the proper visual script nodes.

@Gallilus
Copy link
Contributor Author

Gallilus commented Jan 4, 2022

Will amend tomorow.

@fire
Copy link
Member

fire commented Jan 4, 2022

2022-01-04.13-38-56.mp4

Material for a future or this pull request.

The ability to search for inputs and [sequence] inputs. Outputs already give the correct search.

@Gallilus
Copy link
Contributor Author

Gallilus commented Jan 5, 2022

@akien-mga Happy new year.
And thanks for always having patience with my descriptions and writing.

VBoxContainer *vbc;

Vector<Variant::Type> type_filter;
VBoxContainer *vbox;
Copy link
Member

Choose a reason for hiding this comment

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

Fix later. set to nullptr

Copy link
Member

Choose a reason for hiding this comment

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

Sorry if this was confusing. I mean that the variables should be intitialized to like nullptr or 0, but since there's like 15 variables to check it can be left to another commit.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I concentrated testing on functionality so thanks for catching the commit message and also missed class variable inititializing.

@fire
Copy link
Member

fire commented Jan 9, 2022

@Chaosus Can you take a look?

item->set_selectable(2, false);
item->set_metadata(2, connecting);
void VisualScriptPropertySelector::select_from_visual_script(const Ref<Script> &p_script, bool clear_text) {
set_title(TTR("Select from visual script"));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the "Create Visual Script Node" title sounds better (just to be consistent with "Create Shader Node" in visual shaders).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Create Visual Script Node" is not accurate, it allows you to select a node compatible with the visual script you are editing.
In my mind, Visual script nodes are things like sequences or conditions similar to keywords in GDScript.

@Chaosus
Copy link
Member

Chaosus commented Jan 10, 2022

Not sure whether it is related to this issue but there is a bunch of errors emitted when the node is created from base type(e.g Vector):

vst_bug

@Chaosus
Copy link
Member

Chaosus commented Jan 10, 2022

When there is no text entered in the search text edit it shows only a few keywords:

image

Does this intended? It looks incorrect for me - I expected to watch a full tree or at least all base class Properties + all keywords + all built-in methods.

@Chaosus
Copy link
Member

Chaosus commented Jan 10, 2022

Clicking on category closes a dialog and emits errors:

vst_bug2

Add a check to prevent this from happening.

@akien-mga akien-mga changed the title Update visual_script_property_selector.cpp Improve Visual Script editor to suggest the proper visual script nodes. Jan 10, 2022
@akien-mga
Copy link
Member

WIP for proposal #3528

Is it still WIP or ready? It would be good to amend the OP description if it's now fully implemented.

@Gallilus Gallilus marked this pull request as draft January 11, 2022 19:30
@Gallilus
Copy link
Contributor Author

Next week I will try to fix the errors

@Gallilus Gallilus force-pushed the Update-visual-script-property-selector branch from 7be0e61 to 3a82f66 Compare January 17, 2022 19:23
@Gallilus Gallilus marked this pull request as ready for review January 17, 2022 19:26
@Gallilus Gallilus requested review from Chaosus and fire January 17, 2022 19:26
@Gallilus
Copy link
Contributor Author

Added the null pointers (28)

Fixed the error when dropping nodes.

When there is no text entered there are only a few keywords (visual script nodes) available. Yes else you are provided with too many options and there are no more options for flow control. To see all options press spacebar.

About the error when selecting greyed out options. Yes, it needs to be handled in further PR's. For the above example, it might be interesting to change the scope reference. What do you think people will want when they select a class?

I hope this is a mergeable starting point for further improvements.

@Chaosus
Copy link
Member

Chaosus commented Jan 21, 2022

When there is no text entered there are only a few keywords (visual script nodes) available. Yes else you are provided with too many options and there are no more options for flow control. To see all options press spacebar.

Alright, I'm not a specialist in visual scripts, so it might be ok. I just make a point that pressing the spacebar to show all members is a little bit counter-intuitive - in visual shaders, you see the full tree in the collapsed form at the beginning.

About the error when selecting greyed out options. Yes, it needs to be handled in further PR's. For the above example, it might be interesting to change the scope reference. What do you think people will want when they select a class?

I don't know, maybe you should add a check to ClassDB::is_class_exposed to prevent this and allow instantiate a class, or add a special name setlist to check before pressing.

Nevertheless, I think it's in a good state to merge.

@akien-mga akien-mga merged commit 20dfa0c into godotengine:master Jan 21, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the visual script property selector
5 participants