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 empty script interface crash on tscn load. #23495 #23535

Merged

Conversation

fire
Copy link
Member

@fire fire commented Nov 5, 2018

Add fail conditions to protect the visual script functions and also fix the initiating cause.

@akien-mga Do you prefer if I fix the initiating cause, add the ERR_FAILs or both?

Bugsquad edit: Fixes #23495.

@fire
Copy link
Member Author

fire commented Nov 5, 2018

Noticed an error with visual script function creation adding all VS functions rather than just the node's virtuals.

godot windows tools 64_2018-11-05_13-28-45

I'll make an issue for it.

EDIT: Seems to be specific to the test case in the issue. Investigating.
EDIT: I can't reproduce the issue but I found another issue.

@akien-mga
Copy link
Member

Do you prefer if I fix the initiating cause, add the ERR_FAILs or both?

Well the initiating cause should of course be fixed. As for the ERR_FAIL_CONDs, that's for you to judge whether those methods can be called with invalid input and/or if such invalid input could cause failures. If any of those is yes, then ERR_FAIL_COND checks are warranted.

For _update_members it makes sense to me. For has_function, is there any case where we allow creating (and then testing) for a function with an invalid identifier?

@fire
Copy link
Member Author

fire commented Nov 6, 2018

I think you can name a function anything as a node, so it would be good if has_function didn't crash. However I can think of a case when a function is deprecated, not sure what happens then.

@akien-mga
Copy link
Member

akien-mga commented Nov 6, 2018

I think you can name a function anything as a node, so it would be good if has_function didn't crash.

Would it crash though? has_function only uses Map<StringName, ...>::has(), which can take any kind of StringName and should return false if it's not in the map.

If input validation needs to be done on function naming, it should be done further up when users actually define the name IMO.

@fire
Copy link
Member Author

fire commented Nov 6, 2018

I think my problem is that functions becomes null but that's not solved by the fails check. Functions being null is when scripts is null.

Add fail conditions to protect the visual script function and also fix the initiating cause.
@fire fire force-pushed the empty-script-interface-tscn-23495 branch from 9040eaf to ef78181 Compare November 6, 2018 19:11
@fire
Copy link
Member Author

fire commented Nov 6, 2018

Removed has_function.

@akien-mga akien-mga merged commit 8f39b36 into godotengine:master Nov 6, 2018
@akien-mga
Copy link
Member

Thanks!

@fire fire deleted the empty-script-interface-tscn-23495 branch November 6, 2018 19:32
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.

Switch to the empty script interface and godot forces quit when the tscn scene file with script is loaded
2 participants