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 infinite loop when importing 3D object named "-colonly" #83764

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

timothyqiu
Copy link
Member

@timothyqiu timothyqiu commented Oct 22, 2023

Fixes #83763

The set_owner() seems unnecessary to me. But it's called in other branches where p_node needs to be deleted and return null.

@timothyqiu timothyqiu added this to the 4.2 milestone Oct 22, 2023
@timothyqiu timothyqiu requested a review from a team as a code owner October 22, 2023 03:34
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Seems ok to fix the infinite loop.

This whole code is still in dire need of a major refactoring. Here it should probably not at all handle nodes named -colonly as having a colonly hint.

@akien-mga akien-mga merged commit 4f138db into godotengine:master Oct 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@timothyqiu timothyqiu deleted the suffix-only branch October 26, 2023 15:11
@dc1248
Copy link

dc1248 commented Oct 27, 2023

@akien-mga Are you sure the -colonly node should just be deleted? My impression is that the Blender file has a mesh object with a -colonly child object to be imported as a collision shape. I haven't used that convention but it seems sensible if Godot would allow it.

@akien-mga
Copy link
Member

No, my point is that if a Blender object is just named -colonly, we should either:

  • Consider that it doesn't respect the convention, and thus not apply the import hint. So whatever that object is in Blender would be added as a node in Godot named -colonly, without generating collisions.
  • Consider that it respects the convention, and thus generate a new name for it, likely the default collision polygon node name.

Currently we do a third option which is skipping this node as invalid, with an error. That's also fine given that it's a corner case which is unlikely to happen frequently, and is easy to fix in Blender.

@dc1248
Copy link

dc1248 commented Oct 30, 2023

Okay, fine with me if it prevents needless complexity and bugs. I just wanted to make sure we're not accidentally changing the behavior. I was thinking that nodes created by import hints don't need names because they come in with generated names like @StaticBody3D@19798 and @NavigationRegion3D@19797 as of ~4.1.

It would be nice if someone could actually document these conventions, maybe in the "warning" box on this page... https://docs.godotengine.org/en/latest/tutorials/assets_pipeline/importing_3d_scenes/node_type_customization.html

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.

Importing a .glb with a 3D object in it named "-colonly" crashes Godot
3 participants