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

Importing objects with and without _nav causes node name collision #72483

Open
lyuma opened this issue Feb 1, 2023 · 2 comments
Open

Importing objects with and without _nav causes node name collision #72483

lyuma opened this issue Feb 1, 2023 · 2 comments

Comments

@lyuma
Copy link
Contributor

lyuma commented Feb 1, 2023

Godot version

4.0beta16+0810eca

System information

Windows 10

Issue description

The function in ResourceImporterScene::_pre_fix_node which handles suffixes such as _col or _vehicle or _nav removes the suffix, which can cause a name collision with another node.

Due to the API used (set_name followed by replace_by), it can cause a name conflict to occur.

Elsewhere in the gltf importer, an explicit uniqueness is guaranteed, and additionally Node::add_child(..., true) protects against duplicate names in a better way.

I think it would be better to avoid removing the extension, but this would be compat breaking. Maybe we can add an option in 4.1 or later. (Not removing the extension would reduce the risk of affecting documents in which the "_wheel" naming was accidental, as in this issue: #72419 )

Steps to reproduce

  1. Open the attached project.
  2. New inherited scene on the blend file.
  3. Notice some node names are duplicated nondeterministicly with the @ symbol
    godot_duplicate_invalid_node_names

Minimal reproduction project

Duplicate_Collider_Name_Test.zip

@rburing
Copy link
Member

rburing commented Feb 1, 2023

There actually is logic to prevent the rename (so keep the suffix) if there would be a clash and the suffix is col, convcol, or occ:

if (!fixed_name.is_empty()) {
if (mi->get_parent() && !mi->get_parent()->has_node(fixed_name)) {
mi->set_name(fixed_name);
}
}

But it looks like this was not done in other cases.

I agree life would be simpler if we would not do the renames.

@lyuma
Copy link
Contributor Author

lyuma commented Feb 1, 2023

Reproduces on godot-tests/tests/blend_export too, so this seems like a regression of some sort.

file 32678-respect-import-hints-with-empty.blend
image

Also 43563-asset-collection-test.blend
image

Also a weird thing: colldiers do not remove the suffix. See here it has Marker-col:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants