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

Implement KHR_node_visibility in the GLTF module #93722

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

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jun 28, 2024

Implements KHR_node_visibility KhronosGroup/glTF#2410 in Godot, which allows glTF nodes to be marked as not visible. Both import and export are supported.

Details: The glTF visibility system works the same as Godot's visibility system, only affecting visuals, which makes this really easy to implement. Since this extension was extremely simple, I decided to just implement it in GLTFDocument rather than writing a GLTFDocumentExtension class for it. I also made it a required extension on export, to ensure that user assets are always rendered correctly (EDIT: Required by default, but users can customize this on export). However most scenes don't contain invisible objects, so this won't impact most scenes.

Aside from the GLTF module changes, this PR also adds 2 lines to the general scene importer code to ensure visibility is preserved when converting ImporterMeshInstance3D nodes to MeshInstance3D nodes.

Test file: cube_visibility_example.zip

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I mentioned this proposal at the gltf meeting so I agree with it. Did not test yet.

@fire
Copy link
Member

fire commented Jun 28, 2024

We had a lengthy discussion of the support of the visibility extension if we make it required.

Required extension for something cosmetic is a bad idea IMHO

Required extensions will make godot look bad and create a support headache

@aaronfranke
Copy link
Member Author

Updated the PR to make this configurable on export:

Screenshot 2024-06-28 at 4 14 08 PM Screenshot 2024-06-28 at 4 15 35 PM

modules/gltf/gltf_document.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

Shouldn't we wait for Khronos to approve the draft proposal before merging its implementation? If they reject it, but we merge it, we'd be stuck maintaining a non-standard extension.

@aaronfranke
Copy link
Member Author

@akien-mga Indeed, we can wait for approval. I doubt this will change but I cannot absolutely guarantee that.

@fire
Copy link
Member

fire commented Aug 26, 2024

We discussed on Thursday that the moment Khronos ratifies the extension we are clear to merge.

@aaronfranke
Copy link
Member Author

aaronfranke commented Sep 28, 2024

Marking as draft to prevent accidental merging before ratification. However, the code is complete and ready for review (well, it has already been reviewed, but more review is welcome too).

@aaronfranke aaronfranke marked this pull request as draft September 28, 2024 11:51
@yankscally
Copy link

Honestly, it is only a single bool, its not that deep. and, it has a lot of value to godot when blender and godot share this eye icon in the same panel. I think it's worth supporting and then hotfixing later if Khronos happen to change their mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

5 participants