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

[3.x] Promote object validity checks to release builds #51796

Merged
merged 1 commit into from
Sep 21, 2021

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Aug 17, 2021

Extra:

  • Optimized the debug-only check about why the object is null to determine if it's because it has been deleted (the RC is enough; no need to check the ObjectDB).
  • Because of the previous point. the debugger being attached is not required anymore for giving the "Object was deleted" error; from now, it only matters that it's a debug build.
  • is_instance_valid() is now trustworthy. It will return true if, and only if, the last object assigned to a Variant is still alive (and not if a new object happened to be created at the same memory address of the old one).
  • Replacements of instance_validate() are used where possible Variant::is_invalid_object() is introduced to help with that. (GDScript's is_instance_valid() is good.)

I've smoke-tested this on the Windows editor and on an Android release build of my game. No crashes. No apparent regressions.

Some of all this is still needed in 4.0. That work will happen separately.

Implements godotengine/godot-proposals#1589.

UPDATE: To be honest, I'm not very fond of the changes to Tween so it can robuslty check the validity of the objects involved. I'd like opinions. Fixed.

@RandomShaper RandomShaper added this to the 3.4 milestone Aug 17, 2021
@RandomShaper RandomShaper requested review from a team as code owners August 17, 2021 14:21
@RandomShaper RandomShaper force-pushed the dangling_obj_release_3.x branch from c138235 to 0af7b4c Compare August 17, 2021 15:40
@RandomShaper RandomShaper requested a review from a team as a code owner August 17, 2021 15:40
@RandomShaper RandomShaper force-pushed the dangling_obj_release_3.x branch from 0af7b4c to 6656282 Compare August 17, 2021 16:49
@RandomShaper RandomShaper force-pushed the dangling_obj_release_3.x branch 2 times, most recently from 3d85e19 to 2af4c55 Compare September 20, 2021 20:09
@akien-mga
Copy link
Member

UPDATE: To be honest, I'm not very fond of the changes to Tween so it can robuslty check the validity of the objects involved. I'd like opinions.

The main change seems to be the change of arguments from Object * to Variant. Why is it needed exactly? Since it seems the first thing you do anyway is to cast them to Object *. Does the validation happen only when casting Variants?

@RandomShaper RandomShaper force-pushed the dangling_obj_release_3.x branch from 2af4c55 to 683d99d Compare September 21, 2021 08:30
Extra:
- Optimized the debug-only check about why the object is null to determine if it's because it has been deleted (the RC is enough; no need to check the ObjectDB).
- Because of the previous point. the debugger being attached is not required anymore for giving the "Object was deleted" error; from now, it only matters that it's a debug build.
- `is_instance_valid()` is now trustworthy. It will return `true` if, and only if, the last object assigned to a `Variant` is still alive (and not if a new object happened to be created at the same memory address of the old one).
- Replacements of `instance_validate()` are used where possible `Variant::is_invalid_object()` is introduced to help with that. (GDScript's `is_instance_valid()` is good.)
@RandomShaper RandomShaper force-pushed the dangling_obj_release_3.x branch from 683d99d to 26edc6c Compare September 21, 2021 08:39
@RandomShaper
Copy link
Member Author

Conversion of Variant to Object * in C++ returns null if the object has been released (like the gone decay to null, but only for C++). Therefore, I thought that Tween needed to get Variants so it could do the conversion to Object * itself. However, that was overdoing it, since the binding layer, so to speak, already does the conversion and Tween can just check for null.

The only change needed is to remove the calls to ObjectDB::instance_validate(), since they add no value over the null check. So, that's what I've done in my last push seconds ago.

@akien-mga akien-mga merged commit 22aab6b into godotengine:3.x Sep 21, 2021
@akien-mga
Copy link
Member

Thanks!

@oeleo1
Copy link

oeleo1 commented Sep 21, 2021

@RandomShaper Some commits deserve triple hearts and this is one of them! Thank you. Another was an off by one error in LocalVector fixed by @groud. Looking forward to the next 3.4 release.

@RandomShaper
Copy link
Member Author

@oeleo1, I really appreciate your words. I hope this is indeed as beneficial as we expect!

@oeleo1
Copy link

oeleo1 commented Sep 23, 2021

@RandomShaper Well, this is a game changer indeed. Automatic testing of release builds in 3.1 used to crash within the minute. 3.2 within 15 minutes, 3.4beta5 is still running after 6 hours. That's unseen unless it was a debug build. What a relief... simply fantastic. Bravo!

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.

3 participants