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

Add check before redrawing graph node slot stylebox #65600

Closed
wants to merge 1 commit into from
Closed

Add check before redrawing graph node slot stylebox #65600

wants to merge 1 commit into from

Conversation

JCordara
Copy link

Fixes #65557

Ensure Graph Nodes have more than 0 children before redrawing slot styleboxes. There is already a check in place to ensure the slot's key is not greater than cache_y.size(), but it seems the cache isn't updated before redrawing occurs. Possibly a better fix would be to make sure the cache is updated before redrawing? Not sure, pretty new to the engine still.

@JCordara JCordara requested a review from a team as a code owner September 10, 2022 06:27
@Chaosus Chaosus added this to the 4.0 milestone Sep 10, 2022
Copy link
Contributor

@MJacred MJacred left a comment

Choose a reason for hiding this comment

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

Done with my feedback.

Side note: I'd prefer refreshing slot_info and cache_y first, before issuing a redraw, but not sure what repercussions that has without reading the code more in depth

@@ -392,7 +392,7 @@ void GraphNode::_notification(int p_what) {
}

// Draw slot stylebox.
if (s.draw_stylebox) {
if (s.draw_stylebox && get_child_count() != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion, change line 370 instead

if (E.key < 0 || E.key >= cache_y.size() || get_child_count() == 0) {
	continue;
}

OR add an if-clause before that if-clause in line 370:

if (get_child_count() == 0) {
	break;
}

Reason is: slot_info and cache_y are outdated / dirty, and not yet recalculated

EDIT: if it's desirable to draw slots at least once, even if there are no children, then the 2nd proposal with the break can be added in line 393 instead

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I'll move the check to line 370 as per the first proposal. That was my original idea but I wasn't sure how that might affect other things dependent on the code between 370 and 395. Seems like it's no problem!

@JCordara JCordara requested review from a team as code owners September 10, 2022 17:35
@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

@akien-mga akien-mga requested a review from Geometror September 13, 2022 08:49
@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 19, 2023

Sorry, we apparently had another PR that fixed it, see #69284. Thanks for your contribution nonetheless!

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.

Crash when removing last child node from a GraphNode
5 participants