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

Fix Signal parameters' type will now be correctly displayed in the node signal preview box. #65812

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Life4gal
Copy link
Contributor

Version of Godot based on:
v4.0.alpha.custom_build [20d6672]

Fixes #65809

Signal parameters' type will now be correctly displayed in the node signal preview box.

Modifications to note:

// modules/gdscript/gdscript_parser.cpp
GDScriptParser::parse_signal();

// modules/gdscript/language_server/gdscript_extend_parser.cpp
ExtendGDScriptParser::parse_class_symbol(const GDScriptParser::ClassNode *p_class, lsp::DocumentSymbol &r_symbol);

// modules/gdscript/gdscript.cpp
GDScript::_update_exports(bool *r_err, bool p_recursive_call, PlaceHolderScriptInstance *p_instance_to_update);

Important note: Compatible with 4.x version only.

Preview:
signal_arg_type_1
signal_arg_type_2

If you have questions about non-built-in type errors, please see the modification of modules/gdscript/gdscript_parser.cpp

@Life4gal Life4gal requested a review from a team as a code owner September 15, 2022 03:42
@fire fire changed the title fix-issue-65809-for-4.x Fix Signal parameters' type will now be correctly displayed in the node signal preview box. Sep 15, 2022
@Life4gal Life4gal force-pushed the fix-issue-65809-for-4.x branch from f67b457 to dc733b5 Compare September 15, 2022 03:53
@Chaosus Chaosus added this to the 4.0 milestone Sep 15, 2022
modules/gdscript/gdscript_compiler.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_compiler.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_compiler.cpp Outdated Show resolved Hide resolved
@Chaosus
Copy link
Member

Chaosus commented Sep 15, 2022

Commits need to be squashed to a single one as required by our pipeline (see docs.godotengine.org/en/latest/community/contributing/pr_workflow.html

@Life4gal Life4gal force-pushed the fix-issue-65809-for-4.x branch from 63bc471 to b002d18 Compare September 15, 2022 06:14
modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved
@Life4gal Life4gal force-pushed the fix-issue-65809-for-4.x branch 2 times, most recently from 0d74e62 to c45334c Compare September 15, 2022 06:17
@Chaosus
Copy link
Member

Chaosus commented Sep 15, 2022

[fix-issue-65809-for-4.x]

Please amend the commit name - and rename it to the PR name ("Fix Signal parameters' type will now be correctly displayed in the node signal preview box.") - we don't use names like "fix [issue link]"

@Life4gal
Copy link
Contributor Author

I'm sorry about force-pushed, but due to some previous oversights (forgetting git config), some information that did not belong to me was leaked.
So I used rebase and push -f to get this PR(and commit) updated. :)

@YuriSizov YuriSizov added enhancement and removed bug labels Sep 17, 2022
@YuriSizov YuriSizov modified the milestones: 4.0, 4.1, 4.x Sep 17, 2022
@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 17, 2022

If your goal is to only fix the display in the docks, then this is an overkill. If the typing information is not available to the dock, or is unreliable, we can hide vars or just let them be, as that is still a valid notation. Since the GDScript engine is not doing type validation, any variant type is actually acceptable.

But if you want to actually implement typing support for signals, then this needs to cover all use cases. Namely, Node is a valid type, it's also a built-in type that should be supported. We still have issues supporting custom user-defined types, but this being worked on.

Either way, this is not in scope for Godot 4.0 as GDScript features are no longer accepted (or any enhancements for that matter) and the work should focus on stability and optimization instead. Also, there is already a proposal for this godotengine/godot-proposals#2557 which you might want to consider for your implementation.

@Life4gal
Copy link
Contributor Author

If your goal is to only fix the display in the docks, then this is an overkill. If the typing information is not available to the dock, or is unreliable, we can hide vars or just let them be, as that is still a valid notation. Since the GDScript engine is not doing type validation, any variant type is actually acceptable.

But if you want to actually implement typing support for signals, then this needs to cover all use cases. Namely, Node is a valid type, it's also a built-in type that should be supported. We still have issues supporting custom user-defined types, but this being worked on.

Either way, this is not in scope for Godot 4.0 as GDScript features are no longer accepted (or any enhancements for that matter) and the work should focus on stability and optimization instead. Also, there is already a proposal for this godotengine/godot-proposals#2557 which you might want to consider for your implementation.

See this 😄

@Life4gal
Copy link
Contributor Author

signal_arg_type_validate_4 x_1
😄😄😄

@Life4gal
Copy link
Contributor Author

signal_arg_type_validate_4 x_1 😄😄😄

It looks like signal_name.emit(args...) needs some extra work.

@Life4gal
Copy link
Contributor Author

Life4gal commented Sep 22, 2022

signal_arg_type_validate_4 x_2
😄😄

@Life4gal Life4gal force-pushed the fix-issue-65809-for-4.x branch from 12f8605 to 402ea13 Compare September 22, 2022 12:53
@adamscott
Copy link
Member

CC @vnen

@KoBeWi
Copy link
Member

KoBeWi commented Oct 30, 2024

The linked issue has been resolved. Is any part of this PR still relevant?

@adamscott
Copy link
Member

The linked issue has been resolved. Is any part of this PR still relevant?

It seems that this PR detects when a connected signal signature is incompatible with the connected function.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 5, 2024

Needs rebase then.

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.

[4.x] The node signal preview box does not show the type of the signal parameter.
5 participants