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

Minor enhancements to the GLTF module (lights and docs) #66087

Closed
wants to merge 1 commit into from

Conversation

aaronfranke
Copy link
Member

Minor misc enhancements to the GLTF module. Fix some minor things with lights, rename Ref<GLTFNode> n to gltf_node, give the extensions folder its own SCsub, and add some documentation.

@aaronfranke aaronfranke added this to the 4.0 milestone Sep 19, 2022
@aaronfranke aaronfranke requested a review from fire September 19, 2022 03:34
@aaronfranke aaronfranke requested a review from a team as a code owner September 19, 2022 03:34
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.

Split off from the other pr. Need to wait for cicd.

Edited: #66026

env_gltf.add_source_files(env.modules_sources, "structures/*.cpp")
SConscript("extensions/SCsub")
Copy link
Member

Choose a reason for hiding this comment

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

What's the use for this? It's not done for structures nor editor, if there's no custom config to set for this folder / a sub env, I don't see how it makes things better.

Copy link
Member Author

@aaronfranke aaronfranke Sep 19, 2022

Choose a reason for hiding this comment

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

In isolation it's indeed not better, but it's planning for future PRs where there is more complex logic. In the future I'm planning to have extensions/ to have subfolders in it. If you want I can remove it from this PR, and instead we can introduce this SCsub when it becomes useful.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's fine if you're confident that these future PRs will actually be merged as you envision them. But generally speaking yes, it's best to leave out such future proofing and do the change when it's actually useful.

akien-mga added a commit that referenced this pull request Sep 19, 2022
Minor enhancements to the GLTF module (lights and docs)
@akien-mga
Copy link
Member

I merged a second after you force pushed so it didn't work :)

Merged in 63c0dc6.

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.

3 participants