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

Add is_z_forward option to looking_at() & look_at() and some constants for glTF space #72795

Closed
wants to merge 1 commit into from

Conversation

TokageItLab
Copy link
Member

By convention in OpenGL, objects are often imported facing the camera. This convention has been brought to glTF, so Godot imports glTF assets with +Z as forward.

In environments where this convention exists, there should be two look_at()s, one for the camera and one for the asset.

However, since Godot considers all coordinate systems to be unified by the camera space, so the look_at() and the Vector3 orientation constants are correct for the camera, but not for the glTF asset.

This PR adds some options that assume an imported glTF with +Z as forward. Those should be methods that users have already implemented as the workarounds, it is just not supported by Godot in the core.

In fact, this option is only needed for those that use look_at() and have the forward axis hard-coded, so most methods and classes do not need it and core compatibility is fully preserved.

The only change is regarding PathFollow3D, as the default direction should be the opposite.

@QbieShay
Copy link
Contributor

QbieShay commented Feb 6, 2023

I don't think this is the right solution, like mentioned yesterday we need to discuss and come to a consensus before implementing this.

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 6, 2023

It may not be right for you, but it is not unusual in terms of OpenGL conventions. Yes, there is no consensus yet, so it is still a 4.x milestone, just same as #72753 PR.

@lyuma
Copy link
Contributor

lyuma commented Feb 6, 2023

To be clear, is_gltf would be set for FBX and blend files as well, since those are converted through gltf.

Does this affect OBJ and DAE? What cases is the is_gltf option intended to cover?

Also, the description mentions cameras, but cameras can be imported from gltf as well.

@TokageItLab
Copy link
Member Author

Indeed, since the imported glTF camera will be a Godot camera, is_gltf may be not good. So is_z_forward seems to be common and better.

@TokageItLab TokageItLab changed the title Add is_gltf option to some methods Add is_z_forward option to some methods Feb 6, 2023
_VariantCall::add_variant_constant(Variant::VECTOR3, "GLTF_UP", Vector3(0, 1, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3, "GLTF_DOWN", Vector3(0, -1, 0));
_VariantCall::add_variant_constant(Variant::VECTOR3, "GLTF_FORWARD", Vector3(0, 0, 1));
_VariantCall::add_variant_constant(Variant::VECTOR3, "GLTF_BACK", Vector3(0, 0, -1));
Copy link
Member

Choose a reason for hiding this comment

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

I am in favor of the looking_at changes, but not these. Perhaps this should be a separate PR? I think if we had a PR just for the looking_at changes this would be easier to approve.

Copy link
Member Author

@TokageItLab TokageItLab Feb 7, 2023

Choose a reason for hiding this comment

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

I suppose this is necessary as a basis/reasoning for the is_z_forward option. The documentation also describes glTF forward (+Z).

The +Z concept remains when it comes to importing/exporting assets. For example, it feels strange to specify BACK if you want the model to face forward for retargeting/exporting glTF.

Copy link
Member

@aaronfranke aaronfranke Apr 10, 2023

Choose a reason for hiding this comment

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

I disagree, it's not tied to glTF. Here's a PR with just the look_at changes: #75875

You could have +Z forward when importing an FBX or Collada file for example. Or you could have -Z forward in glTF if you are controlling a glTF camera which has its forward pointing -Z.

Copy link
Member Author

@TokageItLab TokageItLab Apr 10, 2023

Choose a reason for hiding this comment

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

Or you could have -Z forward in glTF if you are controlling a glTF camera which has its forward pointing -Z.

Well, even if the enum GLTF_FORWARD is an exaggeration, it is a SPECIFICATION that all of the assets as a GLTF are +Z FORWARD. A GLTF with a -Z FORWARD is not a GLTF. It is what is called a FAKE GLTF. GLTF cameras are out of scope here. The term "asset" may include the camera, but let's assume here that it generally refers to the object that the camera is shooting at. In other words, it means that the camera takes a picture of the camera.

There may be a better enum (e.g. ASSET_FORWARD), but even if we can't find it, we must document somewhere that as long as Godot uses the GLTF importer for all formats and has a GLTF exporter, the asset must be +Z faced. Even if the word "FORWARD" is not used, it must be made clear that they are to be faced to +Z. (Root motion is also needed that document.)

If you want to argue that assets should be -Z FORWARD in Godot, then the Godot importer and exporter must correctly convert the +Z FORWARD GLTF as -Z FORWARD, but unfortunately this is not accomplished in Godot 4.x was.

scene/2d/path_2d.cpp Outdated Show resolved Hide resolved
@TokageItLab TokageItLab changed the title Add is_z_forward option to some methods Add is_z_forward option to looking_at() & look_at() and some constants for glTF space Feb 7, 2023
@TokageItLab
Copy link
Member Author

Separated the fix for PathFollow. It should be merged by 4.0.

And this PR will need to be merged after reaching consensus after 4.0.

@TokageItLab TokageItLab marked this pull request as ready for review March 1, 2023 20:30
@TokageItLab
Copy link
Member Author

TokageItLab commented Apr 14, 2023

Superseded by #76060

Bugsquad edit: Which was superseded by #76082

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.

6 participants