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

Fix missing renames in the GLTF module #52273

Merged
merged 1 commit into from
Sep 15, 2021

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Aug 31, 2021

These were discovered by @qarmin here: #16863 (comment)

  • translation -> position
  • zfar -> depth_far (changed based on fire's suggestion, was far)
  • znear -> depth_near (changed based on fire's suggestion, was near)
  • type -> light_type

@aaronfranke aaronfranke added this to the 4.0 milestone Aug 31, 2021
@aaronfranke aaronfranke requested review from fire and qarmin August 31, 2021 03:53
@aaronfranke aaronfranke requested a review from a team as a code owner August 31, 2021 03:53
@fire
Copy link
Member

fire commented Aug 31, 2021

zfar -> depth_far
znear -> depth_near 

I don't like losing the z meaning. Other suggestions welcome.

Also I believe these are the exact json names. https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#cameras

@aaronfranke
Copy link
Member Author

Should be working now, sorry it wasn't working when I first opened the PR, I did so a bit too quickly. I used this to test the cameras: https://github.com/KhronosGroup/glTF-Sample-Models/blob/master/2.0/Cameras/glTF/Cameras.gltf

@qarmin
Copy link
Contributor

qarmin commented Sep 1, 2021

@fire what about Camera3D?
Shouldn't far be changed to depth_far and near to depth_near?

@akien-mga akien-mga merged commit 13eff7d into godotengine:master Sep 15, 2021
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the gltf-renames branch September 15, 2021 13:53
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.

4 participants