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: Change "Camera3D" generated node name to "Camera" #81264

Merged
merged 1 commit into from
Sep 16, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Sep 3, 2023

This PR was originally a part of #80270, but fire requested that I split this out.

This string was originally "Camera" but was accidentally renamed to Camera3D during the 4.0 development cycle.

To be clear, this doesn't break compat any more than #80270 does. The names are already changed due to #80270. Before #80270 the names were like "Camera3D2", "Camera3D32", "Camera3D42", etc. As of #80270 they are "Camera3D", "Camera3D2", "Camera3D3", etc. With this PR the names are "Camera", "Camera2", "Camera3", etc. So the names are already changed, this PR just makes it more readable.

@akien-mga
Copy link
Member

The camera node is named Camera3D in Godot 4, it seems sensible to me that we use the same convention for imported scenes, no?

@aaronfranke
Copy link
Member Author

aaronfranke commented Sep 3, 2023

Node names do not necessarily need to match their type. In fact way back in 2019 I suggested having the editor instance nodes like "Camera3D" as just "Camera", but it was unpopular.

Anyway, if this one-line, two-character PR requires debate, I'm just going to close it. It's not important enough.

@lyuma
Copy link
Contributor

lyuma commented Sep 5, 2023

I don't think this is worth the compatibility breakage.

If we are indeed breaking compat by changing node naming behavior in 4.2, I would prefer if somehow this node is always named "Camera" without a number after it. But that's neither here nor there.

@aaronfranke
Copy link
Member Author

aaronfranke commented Sep 5, 2023

@lyuma I don't understand. Without this PR, there is already (very minor) compatibility breakage. Without this PR, the auto-generated nodes will never spit out a node named "Camera". It is here and there.

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.

Approved in meeting

@akien-mga akien-mga merged commit 51f67ea into godotengine:master Sep 16, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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