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 is_instance() helper method to Node #100437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Dec 15, 2024

Adds a helper method to check whether a node is an instance. This allows differentiating intentions between checking empty path and checking for an instance (which implementation-wise is the same thing).

I noticed we have lots of repeated "magic" code for checking things like instances, inheritance, ownership etc. Adding helper methods for such cases makes the code more readable, so I plan to make follow-up PRs with more helpers.

@RandomShaper
Copy link
Member

This may be one of those cases where a seemingly helpful abstraction actually makes code worse to reason about. In some places we have code like this:

if (!node.get_scene_file_path().is_empty()) {
  // Do something with node.get_scene_file_path().
  // (I know there's a path for sure!)
}

After this PR that'd become this:

if (node.is_instance()) {
  // Do something with node.get_scene_file_path().
  // (Hmm... Is there a path or can it be empty?)
}

Maybe it depends on the level of abstraction the code dealing with the Node is working at. However, having the helper method but not universally allowed or recommended would be not ergonomic.

On the other hand, I also find those checks a bit annoying. So, maybe we could find a stronger proxy? For instance, a rewording of is_instance() to something else that more strongly implies there's a path for sure, or using a different concept,

I guess, since this is the beginning of a stream of similar PRs, this warrants a Discussion label.

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 16, 2024

After this PR that'd become this:

It wouldn't, I didn't replace all empty path checks, only these that specifically check for instance.
Examples of code that checks only path:

godot/editor/editor_node.cpp

Lines 1879 to 1880 in b9437c3

if (!scene->get_scene_file_path().is_empty() && _validate_scene_recursive(scene->get_scene_file_path(), scene)) {
show_accept(TTR("This scene can't be saved because there is a cyclic instance inclusion.\nPlease resolve it and then attempt to save again."), TTR("OK"));

if (scene && !scene->get_scene_file_path().is_empty()) { // Only autosave if there is a scene and if it has a path.

if (edited_scene[p_idx].root->get_scene_file_path().is_empty()) {
return TTR("[unsaved]");

Error EditorInterface::save_scene() {
if (!get_edited_scene_root()) {
return ERR_CANT_CREATE;
}
if (get_edited_scene_root()->get_scene_file_path().is_empty()) {
return ERR_CANT_CREATE;

@RandomShaper
Copy link
Member

You've been more clever than me, it seems. 😀 Makes sense.

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.

2 participants