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

GLTF: Add root node export options and GODOT_single_root extension #81851

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Sep 18, 2023

Solves godotengine/godot-proposals#6588

Spec: KhronosGroup/glTF#2329

This PR adds root node export options and a new glTF extension called GODOT_single_root.

This was discussed with @lyuma about 2 months ago, but was on hold for most of that time due to me waiting for other PRs to be merged as prerequisites (and me not wanting to open draft PRs with a long dependency chain).

Screenshot 2023-09-18 at 1 41 22 AM

(with this PR by itself, there is no export settings dialog, but it can be set through GDScript code when exporting from script, and the goal is to make this available in the export settings once that's added)

  • Single Root: Treat the Godot scene's root node as the root node of the glTF file, and mark it as the single root node via the GODOT_single_root glTF extension. This will be parsed the same as Keep Root if the implementation does not support GODOT_single_root.
  • Keep Root: Treat the Godot scene's root node as the root node of the glTF file, but do not mark it as anything special. An extra root node will be generated when importing into Godot. This uses only vanilla glTF features. This is equivalent to the behavior in Godot 4.1 and earlier.
  • Multi Root: Treat the Godot scene's root node as the name of the glTF scene, and add all of its children as root nodes of the glTF file. This uses only vanilla glTF features. This avoids an extra root node, but only the name of the Godot scene's root node will be preserved, as it will not be saved as a node.

The last 2 options both use only vanilla glTF features. Keep Root is the same as 4.1. Multi Root is a new feature, which allows round-tripping a scene without an extra root node, but the downside is that the root node will only ever be a Node3D because the Godot scene's root node is only being saved as a name instead of a node.

The first option, Single Root, uses the GODOT_single_root extension. This extension has no data, it is only a flag with rules and restrictions. Single Root is identical to Keep Root except that it adds this one string to the "extensionsUsed" array to mark it as the single root node. Godot will read this flag and set the glTF root as the single root node of the generated Godot scene. This flag will do nothing in implementations that don't support it, so in those cases it behaves identically to exporting with Keep Root, and exporting in 4.1 and earlier.

@fire
Copy link
Member

fire commented Sep 21, 2023

@aaronfranke Said he will write a proposal at the review meeting we had today.

@aaronfranke
Copy link
Member Author

@fire @lyuma Proposal opened godotengine/godot-proposals#7791

Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just concerned that there might be a case where the propagate_call("set_owner") could print an error.

Comment on lines 5892 to 5895
Array args;
args.append(p_scene_root);
current_node->propagate_call(StringName("set_owner"), args);
Copy link
Contributor

Choose a reason for hiding this comment

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

An if statement got lost here.

It's important that the call to set_owner specifically still be guarded with a check that (current_node != p_scene_root)

Otherwise if you call scene_root.set_owner(scene_root) it will print an error or malfunction.

If you are 100% certain that current_node is never the same as p_scene_root in any of the 3 export modes, it would help if you wrote an explanation because it feels like it could happen especially in the single root case.

Copy link
Member Author

@aaronfranke aaronfranke Sep 22, 2023

Choose a reason for hiding this comment

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

It wasn't lost, just changed. It won't ever happen because p_scene_root will either be null or different from current_node. When generating the scene root node, we pass in null to this function.

I already have a comment above "If the root node argument is null, this is the root node", is this not enough?

Copy link

@Scoppio Scoppio left a comment

Choose a reason for hiding this comment

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

Not gonna lie, this may very well come in handy later for me heheheh

I can confirm it working on Windows 10.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Sep 26, 2023
@akien-mga akien-mga merged commit dc14f02 into godotengine:master Sep 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the gltf-single-root branch September 26, 2023 16:55
@akien-mga akien-mga changed the title GLTF: Add root node export options and GODOT_single_root extension GLTF: Add root node export options and GODOT_single_root extension Oct 3, 2023
@fire
Copy link
Member

fire commented Sep 14, 2024

@aaronfranke Do you think it'll be doable to write a gltf spec for GODOT_single_root I can present / pull request?

@aaronfranke
Copy link
Member Author

@fire I have already done this: KhronosGroup/glTF#2329

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.

5 participants