Conversation
Reviewer's Guide by SourceryThis pull request updates the Godot version to 4.3, which introduces changes in how graph elements are handled. The main modifications are in the File-Level Changes
Tips
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @atomic-junky - I've reviewed your changes - here's some feedback:
Overall Comments:
- Could you provide more context on the specific issues this PR is addressing? The description mentions breaking bugs in Godot 4.3, but it's not clear how these changes relate to those issues.
- The introduction of
get_nodes()seems to be a significant change. Can you explain the reasoning behind this and whether you've considered any potential performance implications, especially if this method is called frequently?
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
There was a problem hiding this comment.
Hey @atomic-junky - I've reviewed your changes - here's some feedback:
Overall Comments:
- Could you provide more context on the specific issues this PR is addressing? The description mentions breaking bugs in Godot 4.3, but it's not clear how these changes relate to those issues.
- The new
get_nodes()function usesfilter(), which could potentially impact performance if called frequently. Consider the performance implications and possibly optimize if necessary. - The addition of a type check in
_on_node_selectedsuggests that non-MonologueGraphNode objects might be passed to this function. This could indicate a design issue that needs addressing.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
The main bug is when you call get_children() (only tested on GraphEdit) it also gives you internal childrens. |
RailKill
left a comment
There was a problem hiding this comment.
Static Typing
I think using .filter() as a one-liner is neat, but functional programming in Godot foregoes static typing which isn't good for maintainability. It's as you say, Godot doesn't understand that. This is optional but I recommend a more verbose option which isn't as neat but does the same thing, if you'd like:
func get_nodes() -> Array[MonologueGraphNode]:
var list: Array[MonologueGraphNode] = []
for node in get_children():
if node is MonologueGraphNode:
list.append(node)
return list
Crash in Test
I tried running Test with MrSharpener example, when it tries to exit back to the Test main menu, it throws an error on Main.gd line 73. Can you check if this happens on your side too?
Not sure what's the actual cause, seems like there's some cyclic dependency that's being caught in Godot 4.3 here. The easiest way to get around this problem is to just change from preload() to load() for the menu scene. If you want to fix the cyclic dependency itself, I think it probably has something to do with the option_button.tscn in the menu scene. Up to you how you'd like to fix this.
If you can run Test without crashing, this PR should be good to merge 👍
|
Done! |
Godot 4.3 adds many new features/changes, especially with graph elements but also comes with breaking bugs.
Summary by Sourcery
Update the script to use a new method get_nodes() that filters children nodes to only include MonologueGraphNode instances, improving the handling of nodes in the graph.
Enhancements: