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: Preserve node visibility on import #98874

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Nov 6, 2024

KHR_node_visibility is implemented by PR #93722 but is not yet ratified by Khronos. In the meantime, I tried to implement this in GDScript, but I found that it was impossible for 2 reasons:

  • Visibility is not preserved when moving from ImporterMeshInstance3D to MeshInstance3D during import.
  • Invisible nodes are discarded during export.

This PR extracts the fixes to these problems the first problem from PR #93722 to allow implementing KHR_node_visibility in GDScript. EDIT: For import only.

The fix for not discarding invisible nodes may be controversial, especially since due to the lack of the user-facing settings in PR #93722, there is no exposed setting to customize this. Still, I think this is a good change. I would exclude this part when cherry-picking to 4.3 though. Or, I can remove it if I really must, and at least we can fix importing.

@aaronfranke aaronfranke added this to the 4.4 milestone Nov 6, 2024
@aaronfranke aaronfranke requested a review from a team as a code owner November 6, 2024 02:40
@fire
Copy link
Member

fire commented Nov 6, 2024

I would move the not discarding invisible nodes to the node visibility pr unless you want to do the full forward compatibility toggle dance. It's up to you.

We can do it now or later.

@aaronfranke
Copy link
Member Author

@fire Updated the PR to remove the "not discarding invisible nodes" part.

@aaronfranke aaronfranke added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 6, 2024
@fire
Copy link
Member

fire commented Nov 6, 2024

We discussed:

The problem with adding a user-facing option is that the three options should be "Include & Required," "Include & Optional," and "Exclude."

Without KHR_node_visibility, the first two would behave the same.

This causes an option that would change meaning in a future release.

Another option is that I could add in every bit of code except the reading/writing to GLTF part, making it trivially easy for people to implement KHR_node_visibility in GDScript, but it would be mostly unused code out of the box.

I do not favour this because it doesn't default on, so any bugs will be hidden for maybe a year.

@aaronfranke aaronfranke changed the title GLTF: Preserve node visibility on import and keep on export GLTF: Preserve node visibility on import Nov 6, 2024
@fire fire requested a review from a team November 6, 2024 03:08
@fire
Copy link
Member

fire commented Nov 6, 2024

The artifact upload failed, so feel free to press the redo failed jobs button.

@aaronfranke aaronfranke removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 6, 2024
@aaronfranke
Copy link
Member Author

The cherrypick was not trivial so I removed the lables and opened PRs #98877, #98878, and #98879.

@Repiteo Repiteo merged commit 3a7efc9 into godotengine:master Nov 7, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 7, 2024

Thanks!

@aaronfranke aaronfranke deleted the gltf-preserve-visibility branch November 7, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants