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 use_model_front option to look_at in Basis, Transform3D, and Node3D #75875

Closed
wants to merge 1 commit into from

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Apr 10, 2023

EDIT: I modified this PR to do the same thing as #72842 but without the path/curve changes, and with some documentation tweaks. Old description collapsed:

This PR is a subset of #72795, split into its own PR for easy reviewing. This adds the is_z_forward option to look_at in Basis, Transform3D, and Node3D. It's mostly the same as @TokageItLab's code except:

  • I changed the documentation to be more clear and to not presume the reader has knowledge of glTF.
  • This PR does not include the GLTF_* constants which I don't think are desirable to have in the core engine.
    • Note that glTF itself is not self-consistent, the spec defines the camera's forward as -Z and asset forward as +Z. Also some glTF files may have -Z as forward to better align with the camera forward. Also other files like FBX may have +Z forward assets. So calling +Z forward "glTF forward" is not sensible.

I think as long as we agree we want to have this option it's easy to see that this PR is good. Also, this solves #27544.

@TokageItLab
Copy link
Member

TokageItLab commented Apr 10, 2023

Also some glTF files may have -Z as forward

I despise those who do this. It is an act of rebellion against the standardization of formats and an act of aggression against Khronos.

If you want glTF assets to be -Z, then you should file a complaint with the Khronos group, instead of airing your grievances in the Godot repository.

@Zireael07
Copy link
Contributor

@TokageItLab Sorry to say, but what you feel about the use of -Z as forwards has no bearing on the fact that there ARE assets that use -Z. You're free to avoid them like a plague, and not to use this option, after all it's optional.

Personally I've come across those assets more than a couple of times, and getting them to work is a bear. +1 to the PR.

@TokageItLab
Copy link
Member

TokageItLab commented Apr 10, 2023

It is okay for you privately to use assets that are facing the opposite direction, but you should never promote it around by saying these assets are glTF.

The orientation and coordinate system of assets was not clear in fbx, and may have been in the beginning in OpenGL as well. So Khronos created glTF, a format for standardization, which has +Z Forward as a specification (https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#coordinate-system-and-units).

Again, this is NOT a RECOMMENDATION, it is a SPECIFICATION.

Surely if we consider the camera space to be consistent with the coordinate system of the whole system, then OpenGL is -Z Forward, and there a +Z Forward glTF asset might be facing backward. Although this was a convention, but it can be said that glTF is a redefinition of it as a specification.

So having a workflow like #72753 as an optional Addon/Plugin is fine, but to say that glTF can be -Z Forward is completely wrong and nonsense.

Repeatedly, if Godot import and export correctly converts a "+Z Forward glTF Asset" to a "-Z Forward Godot Asset", then I would not complain about the asset being -Z Forward in Godot, and @aaronfranke would not complain about the asset being +Z Forward in glTF.

But unfortunately we can't implement this in Godot 4.x already, so we will have to wait for Godot 5. I would be willing to help accomplish that, but right now we should not fight about it here in order to stabilize Godot 4.

One thing that can be said here is that "you should not overstep the glTF specification to try to force Godot 4 to solve it".

@YuriSizov
Copy link
Contributor

YuriSizov commented Apr 10, 2023

I would like to remind everyone that we are working on a convenience tool for game developers, not on a standard implementation of the spec. As such, a more generic option that is clear in what it does and that can be applied without prejudice is better than another approach.

In a sea of possibilities with how creators make their assets, with inconsistent formats and inconsistencies between different formats — a simple and straight-forward option "Is Z forward" provides a clear and convenient tool, IMO. It's no different from having an up vector in all the same methods. And we leave this tool in the hands of the user, not in our hands, to use as they find appropriate.

@TokageItLab
Copy link
Member

TokageItLab commented Apr 10, 2023

@YuriSizov I have no objection to this PR being merged, since I originally created it to handle both -Z Forward +Z Forward cases and this PR is just removed the glTF enum from it. But it should not be based on the falsehood that glTF can be -Z Forward. I believe it should be noted in the pipeline documentation or somewhere that glTF should be +Z Forward.

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

So I won't block this, but would like to update on the pipeline documentation.

@aaronfranke
Copy link
Member Author

aaronfranke commented Apr 10, 2023

To be clear, look_at is a method in the core engine. It's not related to glTF or its standards like forward direction.

The current behavior is to only use -Z as forward, so it currently only does the opposite of what @TokageItLab wants. This PR adds the option to use +Z as forward.

I'm puzzled by how much heated discussion is going on. You want it to be only one way, but that way is the opposite of how it currently is, and this PR is just adding an option to make your way possible.

@TokageItLab
Copy link
Member

TokageItLab commented Apr 10, 2023

Yes, I definitely want this option implemented and this PR merged.

However, I am opposed to remove the fact that the glTF asset has the specification that it is +Z Forward from the documentation and not explaining it anywhere. We need to write that somewhere, like in the pipeline workflow from Blender to Godot.

And I finally approved this for now since it doesn't have to be in this PR.

@Zireael07
Copy link
Contributor

Totally with you on explaining the GLTF default in docs, but...

I am opposed to remove the fact

uh, where is anyone REMOVING a fact from the docs?

@TokageItLab
Copy link
Member

TokageItLab commented Apr 10, 2023

@Zireael07 The original PR #72795 defined an enum called GLTF_FORWARD to describe the forwards in glTF assets, but this PR removes them.

@aaronfranke may have created this PR as a means of avoiding discussion of those enums, but I recommend that glTF forwards should be explained, even if there are no enums. And I remember in the discussion @aaronfranke wanted to allow -Z Forward in glTFs and he complained that retargeting does not work in such a fake glTF.

To avoid more such complains, I recommend that we need to make users know the correct asset orientation for import/export somewhere, but as I mentioned above, that can be done outside of this PR.

Of course, at worst it could be explained as "glTF asset must be oriented world Vector3.BACK in Godot in gl coordinate system when importing/exporting". Anyway, I want to make it clear to all 3D users that the glTF assets must be +Z Forward and it is specification.

@aaronfranke
Copy link
Member Author

@Zireael07 What Tokage means is that this PR is not adding it, while his PR was adding it.

Indeed it should be documented somewhere, but not in core. Maybe here.

@Zireael07
Copy link
Contributor

@aaronfranke Thanks for clarifying. Yeah, things like this should be documented, preferably either where 3D assets are mentioned or under importing.

@TokageItLab
Copy link
Member

Superseded by #76060

@aaronfranke
Copy link
Member Author

aaronfranke commented Apr 14, 2023

Re-opening because this is still being discussed, and I still prefer this solution. We can re-close if #76060 ends up being merged. EDIT: Will likely be superseded by #76082 instead. EDIT 2: Nope it was #72842.

@aaronfranke aaronfranke changed the title Add is_z_forward option to look_at in Basis, Transform3D, and Node3D Add use_model_front option to look_at in Basis, Transform3D, and Node3D May 24, 2023
Co-authored-by: Juan Linietsky <reduzio@gmail.com>
Co-authored-by: Aaron Franke <arnfranke@yahoo.com>
@akien-mga
Copy link
Member

Superseded by #72842.

@akien-mga akien-mga closed this May 24, 2023
@aaronfranke aaronfranke deleted the lookat-z-forward branch May 24, 2023 09:09
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