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

Set base_path and filename during GLTF export when writing to a file #79636

Merged
merged 2 commits into from
Aug 4, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jul 18, 2023

This PR sets base_path and filename during the export process, and exposes filename. Previously only base_path was exposed and it was only used during the import process.

🚲🏚️: I used file_name for consistency with most of the engine's file_path, but as for just file names and not paths filename is twice as common as file_name. Personally I prefer file_name.

@fire
Copy link
Member

fire commented Aug 1, 2023

I rather not add to the inconsistency. Any suggestions? @YuriSizov

@lyuma
Copy link
Contributor

lyuma commented Aug 2, 2023

Approved in terms of technical review.

I'll leave bikeshedding of file_name up to others

@YuriSizov
Copy link
Contributor

In the public API filename is used multiple times and file_name is used never (it is only ever found in docs in a code sample of DirAccess as a local variable name). So for consistency it should be filename.

@aaronfranke aaronfranke force-pushed the gltf-file-name-path branch from 3812827 to 97fd5d5 Compare August 2, 2023 18:33
@aaronfranke aaronfranke changed the title Set base_path and file_name during GLTF export when writing to a file Set base_path and filename during GLTF export when writing to a file Aug 2, 2023
@akien-mga akien-mga changed the title Set base_path and filename during GLTF export when writing to a file Set base_path and filename during GLTF export when writing to a file Aug 3, 2023
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 3, 2023
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Needs a rebase though.

Also, and this is not new to this PR, but shouldn't getters be const for GLTFState members? (Note that changing that would require adding compatibility methods for the GDExtension API.)

@aaronfranke aaronfranke force-pushed the gltf-file-name-path branch from 97fd5d5 to 2970839 Compare August 3, 2023 21:52
@aaronfranke
Copy link
Member Author

@YuriSizov Rebased and re-tested. Thanks for the catch with GLTFState property constness. Since filename is being added in this PR and copyright is not in Godot 4.1, I have changed those to be const. But base_path and the other properties were already exposed in Godot 4.1 so those will have to wait for the future to be changed to const.

@akien-mga
Copy link
Member

But base_path and the other properties were already exposed in Godot 4.1 so those will have to wait for the future to be changed to const.

They could be changed now, you then just need to add compatibility methods so that existing compiled GDExtensions can find the old method hash and keep using it. See examples in #80168.

@akien-mga akien-mga merged commit e605a1d into godotengine:master Aug 4, 2023
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the gltf-file-name-path branch August 4, 2023 15:03
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