-
Notifications
You must be signed in to change notification settings - Fork 31
Multiline and renaming #82
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
Conversation
wnbaum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Looks a lot cleaner! I suppose we don't even need SignalManager anymore... I made that more complicated than I needed to hah.
See comments for small changes.
| b.block_format = "Send signal {signal: STRING} to node {node: NODE_PATH}" | ||
| b.statement = 'if get_tree().root.has_node("SignalManager"):\n' + '\tget_tree().root.get_node_or_null("SignalManager").send_signal_to_node({node}, {signal})' | ||
| b.block_format = "Call method {method_name: STRING} in node {node_path: NODE_PATH}" | ||
| b.statement = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generates code that looks like this:
func _on_body_entered(_body: Node2D):
var body: NodePath = _body.get_path()
var node = get_node(body)
if node:
node.call('test')
We should probably have both the signal and the "call method in node" block work with TYPE_OBJECT instead of TYPE_NODE_PATH. We can do this in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. After all the type discussions I realize that we should have 2 types of Value blocks:
- Variant block
- Node block
The Value node blocks can have a hint like "Node2D" for parameters. Basically the PROPERTY_HINT_NODE_TYPE in global scope. But yes let's think about this as a group after the MVP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generates code that looks like this:
func _on_body_entered(_body: Node2D): var body: NodePath = _body.get_path() var node = get_node(body) if node: node.call('test')We should probably have both the signal and the "call method in node" block work with
TYPE_OBJECTinstead ofTYPE_NODE_PATH. We can do this in another PR.
By the way is this something that have regressed? I think it was like this before the multi-line change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, yes, that would be good. Maybe we don't have to make a new block since a node block is really a value too? We could just add a new property to value block or extend it.
We'll discuss!
| await get_tree().root.ready | ||
| """ | ||
| . dedent() | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dedent() is nice! Didn't know about it but would have done it this way if I had :)
| ) | ||
| % [verb] | ||
| ) | ||
| b.signal_name = "body_%s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| b.signal_name = "body_%s" | |
| b.signal_name = "body_%s" % [verb] |
Don't forget the string replacement here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch!
This is more readable. Even with the extra parenthesis added by the gdformat formatter.
The rest of the blocks match the GDScript nomenclature. Except for a few "simple" or higher level abstractions. This also removes the signal manager. Keep the await for scene ready in a separate block so it can be used explicitly when needed. Add it to the Lifecycle category. Keep the category "Signal" for now as they need overhaul in ordering and colors.
This is not needed anymore. This breaks the Pong demo, but it will be redone soon when things stabilize.
69a745e to
124a7cf
Compare
Yes, the only reason I left it was to avoid breaking the Pong demo. But it has to be redone anyways as the blocks changed, categories changed, etc. |
wnbaum
left a comment
There was a problem hiding this 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 now!
No description provided.