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

GDScript: Partially allow member lookup on invalid scripts #92609

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

rune-scape
Copy link
Contributor

@rune-scape rune-scape commented Jun 1, 2024

this was supposed to be my fix for this regression: #92534
it does fix it separately from #92544

rethinking the changes i made in #92035 and #90601

  • always default initializes static variables right after compiling a class, safer and less restrictive
  • doesn't invalidate script when dependent scripts don't compile/resolve, as this may have caused other (non segfault) problems i didn't imagine
  • each GDScriptFunction on invalid scripts are protected with a check to see if the script is valid instead of returning too early

this all means that invalid scripts can still have their members safely queried and possibly even native base functions called if there are instances with the invalid script attached
and that a script is not 'invalid' if only a depended script is 'invalid' (as it was before)

fixes #92610

+ always default initialize static variables
+ dont invalidate script when dependant scripts don't compile/resolve
@rune-scape rune-scape force-pushed the rune-relax-gds-guards branch from 528bf64 to 7f7114c Compare June 2, 2024 09:09
@akien-mga akien-mga changed the title GDScript: partially allow member lookup on invalid scripts GDScript: Partially allow member lookup on invalid scripts Jun 3, 2024
@akien-mga akien-mga requested review from adamscott and dalexeev June 19, 2024 08:57
@CsloudX
Copy link

CsloudX commented Jun 21, 2024

Thanks, i'm tested with this pull, it fixed #92610


return E->value->call(nullptr, p_args, p_argcount, r_error);
return E->value->call(nullptr, p_args, p_argcount, r_error);
}
}
top = top->_base;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how critical this is, but there may be a situation where the current script is not yet valid, but the base script is already valid. In this case, static function lookup will work inconsistently for "inherited" (actually shadowed) static methods. Until the current script compiles successfully, the lookup will find the base method, and then the current class method.

@bitwes
Copy link

bitwes commented Jun 27, 2024

I tested this PR with GUT and it fixed the issue.

Minor note
In an overly complicated scenario (which I can describe if necessary), I had to add an additional check of is_instance_valid where I was previously just checking for null. Without this check I get Attempt to call function 'get_current_test_object' in base 'previously freed' on a null instance.

For all I know, this could be an issue with GUT that was fixed in Godot, and not a Godot issue. It's a non issue for GUT, I am just documenting my experience with this PR.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Makes sense and looks good in theory, but TIWAGOS

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

As discussed in the release management meeting yesterday, let's go ahead with that change even though GDScript contributors are not available right now for an in-depth review.

This is a follow up to rune's own changes and was tested by several affected users, so it should be good to go and test more widely in the next beta.

@akien-mga akien-mga merged commit a31525c into godotengine:master Jun 28, 2024
16 checks passed
@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.

Invalid call. Nonexistent function 'reload' in base 'GDScript'.
6 participants