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 import and saving related crashes #89780

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 22, 2024

  • Don't add empty mesh to result when importing obj files
  • Check for null resources in ResourceSaver

This adds the check from core_bind to ResourceSaver directly, could remove the one in ResourceSaver.save in core_bind as it's now redundant, but just added it for now, it helps catch broken cases when doing things from c++

The obj side avoids doing any of the remaining code, makes things a bit clearer, and matches the code here:

if (mesh->get_surface_count() > 0) {
mesh->set_name(name);
r_meshes.push_back(mesh);
mesh.instantiate();
}

For this specific case we might want to look at adding support for lines in obj files as they support it, or a more clear error message or warning when using unsupported features, but that's outside the scope of this


@AThousandShips AThousandShips added bug topic:import crash cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 22, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Mar 22, 2024
@AThousandShips AThousandShips requested review from a team as code owners March 22, 2024 11:40
@akien-mga
Copy link
Member

This adds the check from core_bind to ResourceSaver directly, could remove the one in ResourceSaver.save in core_bind as it's now redundant, but just added it for now, it helps catch broken cases when doing things from c++

I would remove the redundant core_bind checks then, as it seems to be wasteful for performance to do these checks twice.

* Don't add empty mesh to result when importing obj files
* Check for null resources in `ResourceSaver`
@AThousandShips
Copy link
Member Author

Done!

return ::ResourceSaver::save(p_resource, p_path, p_flags);
}

Vector<String> ResourceSaver::get_recognized_extensions(const Ref<Resource> &p_resource) {
ERR_FAIL_COND_V_MSG(p_resource.is_null(), Vector<String>(), "It's not a reference to a valid Resource object.");
Copy link
Member Author

Choose a reason for hiding this comment

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

This one isn't entirely redundant as it exits a bit faster, but it's only skipping over the loop which is minimal

@akien-mga akien-mga merged commit 0cf1557 into godotengine:master Apr 22, 2024
16 checks passed
@AThousandShips AThousandShips deleted the import_fix branch April 22, 2024 10:59
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release crash topic:import
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importing a line only obj crash Godot Editor
2 participants