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

Reorganize the meaning of engine axes. #76060

Closed
wants to merge 1 commit into from

Conversation

reduz
Copy link
Member

@reduz reduz commented Apr 14, 2023

Until not too long ago, Godot did not really have any concept of forward, back, etc. As the push to make the engine easier to use grew, meanings for each axis unit vector were added to the engine.

The problem, however, is that Godot is an engine that uses the right handed coordinate system with Y as "up axis". Unlike engines that use the Left Hand Rule (Unity, Unreal), this causes a conflict between what the camera considers as forward (Z-) and what imported 3D models end up considering as forward when converted to this coordinate system (Z+). As such, the use of methods such as look_at() or constants such as Vector3.FORWARD do not yield the expected results, as the meaning of foward differs depending on the context.

image

Again, this is not a problem in engines such as Unity due to the chosen coordinate system (which followed the Direct3D standard), where forward is the same for both cameras and models, but it is an explicit problem for Godot due to its chosen coordinate system (which followed the OpenGL standard).

As it is obviously more than a decade too late to change the coordinate system in Godot, this PR intends to ameliorate confusion for Godot users, by explicitly separating the concept of camera forward and model front, and deprecating unique constants that can be ambiguous but conflicting in meaning.

Additionally, this PR ensures that engine axes are renamed to things that make more sense, as now Vector3::FORWARD is Vector3::CAMERA_FORWARD (pointing to Z-, which confused many users). And a Vector3::FRONT is added that is Z+.

This PR also requires #76052 being merged to ensure everything is fully consistent again.

Copy link
Contributor

@QbieShay QbieShay left a comment

Choose a reason for hiding this comment

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

LGTM. Since the inconsistency is specifically between what is a camera and what isn't, i think the extra parameter should be called from_camera (the "phrase it creates is look at from camera or look at not from camera). This way users don't need to have the extra knowledge of knowing that camera use -z, they can just answer the "from camera" question.

@reduz
Copy link
Member Author

reduz commented Apr 14, 2023

@QbieShay I am not sure I get which line you refer to, I think you mean in Basis?

scene/3d/node_3d.cpp Outdated Show resolved Hide resolved
@QbieShay
Copy link
Contributor

@QbieShay I am not sure I get which line you refer to, I think you mean in Basis?

Yep sorry, i thought i commented directly on the code.

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.

I approve this PR as one of the people who worked on the animation and glTF pipeline workflow. I almost agree with reduz.

There has been a lot of discussion about having two directions, but I believe we cannot to avoid this way during Godot 4.x, so we need to put an end to the -Z/+Z Forward issue today. I can't have idea what kind of requests we will get in Godot 5 until after this is merged.

After this PR is merged, a more detailed article about pipeline is needed on the document or blogpost.

Also, a same option needs to be added to #72842 (That PR doesn't pass the test as there is no constant link to the class-reference yet).

@reduz reduz force-pushed the reorganize-axis-meanings branch from abf87e8 to 1e3c28b Compare April 14, 2023 15:21
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
@reduz reduz force-pushed the reorganize-axis-meanings branch 2 times, most recently from 834bf5b to 47378c9 Compare April 14, 2023 17:32
core/variant/variant_call.cpp Outdated Show resolved Hide resolved
scene/3d/node_3d.cpp Outdated Show resolved Hide resolved
scene/3d/node_3d.cpp Outdated Show resolved Hide resolved
@reduz reduz force-pushed the reorganize-axis-meanings branch 2 times, most recently from 3f68d49 to cacd543 Compare April 14, 2023 18:14
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
@reduz reduz force-pushed the reorganize-axis-meanings branch from cacd543 to 7f100a0 Compare April 14, 2023 19:12
Until not too long ago, Godot did not really have any concept of *forward*, *back*, etc.
As the push to make the engine easier to use grew, meanings for each axis unit vector were added to the engine.

The problem, however, is that Godot is an engine that uses the [right handed coordinate system](https://en.wikipedia.org/wiki/Right-hand_rule) with Y as "up axis".
Unlike engines that use the Left Hand Rule (Unity, Unreal), this causes a conflict between what the camera considers as *forward* (Z-) and what imported 3D models end up considering as *forward* when converted to this coordinate system (Z+).
As such, the use of methods such as *look_at()* or constants such as **Vector3.FORWARD** do not yield the expected results, as the meaning of foward differs depending on the context.
This is not a problem in engines such as Unity due to the chosen coordinate system, where *forward* is the same for both cameras and models.

As it is obviously more than a decade too late to change the coordinate system in Godot, this PR intends to ameliorate confusion for Godot users, by explicitly separating the concept of *camera forward* and *model front*, and deprecating unique constants that can be ambiguous but conflicting in meaning.
@timothyqiu
Copy link
Member

timothyqiu commented Apr 15, 2023

Again, this is not a problem in engines such as Unity due to the chosen coordinate system (which followed the Direct3D standard), where forward is the same for both cameras and models, but it is an explicit problem for Godot due to its chosen coordinate system (which followed the OpenGL standard).

My nitpick is that coordinate system choice should not be the one to be blamed here.

The root cause is that we imported glTF models as-is. glTF stores the model +Z forward, -X right, while Godot uses -Z forward, +X right.

A model facing forward in Blender is exported as facing +Z in glTF. But Godot keeps the imported model facing +Z in the imported scene, which is backward facing.

  • So the front view is viewing the model's butt.
  • So the look_at() function doesn't work for models.

The ideal solution is to convert the coordinate system when importing. i.e. instead of simply rotating the model 180 degrees, vector (1, 0, 1) should be transformed into (-1, 0, -1). But this would also require large modifications to systems like animation and is probably compat breaking. So it's too late to do things the correct way.

So we decided to keep the imported model as-is last night, and:

  • the front view is viewing the model's butt: flip the front view and back view.
  • the look_at() function doesn't work for models: add an extra argument to look using the +Z axis.

I think we must admit that this is a workaround for a problem that we can't afford to fix correctly.

@reduz
Copy link
Member Author

reduz commented Apr 15, 2023

@timothyqiu I understand this point of view, but if you look at virtually any other software that uses this coordinate system, it uses the imported models the same way, so besides the fact that it's not a great idea at this point of time to innovate by importing the models flipped and flipping all the camera management in the editor, its also not a good idea IMO to create yet another different coordinate system for models (which would be something like RHS, Y-up, flipped 180 in Y).

@Iniquitatis
Copy link
Contributor

I think we must admit that this is a workaround for a problem that we can't afford to fix correctly.

If this is all about models, can't there be an option (maybe recommended or enabled by default until the next major release) to simply flip models on import? This way there would be no need to introduce more band-aids to the coordinate system manipulation that certainly won't make the engine easier to learn/use for newcomers, while the existing users could gradually move their projects towards this model flipped approach. Switching brain between two modes isn't something that I'd call errorproof.

@TokageItLab
Copy link
Member

@Iniquitatis In Godot 4 which uses the right-hand coordinate system, the model and camera have different forwards, which is the canonical way to do it, so I believe there is no chance for that.

If you want the model and the camera forward to be the same, and you believe that is the right thing to do, then you need the proposal of adopting a left-handed coordinate system in Godot 5.

For those who really want to hack reproduce the left-hand coordinate system by rotating the imported model to -Z, maybe an only possible work-around is to make #72753 an add-on and only enable the add-on to rotate the model by default for those who have enabled it.

<param index="0" name="target" type="Vector3" />
<param index="1" name="up" type="Vector3" default="Vector3(0, 1, 0)" />
<description>
Rotates the node so that the [constant Vector3.MODEL_FRONT] axis (+Z) points toward the [param target] position. Use this method when you want to point a 3D model towards a specific position. For 3D cameras ([Camera3D]) use [method camera_look_at].
Copy link
Contributor

Choose a reason for hiding this comment

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

Vector3.FRONT

@QbieShay
Copy link
Contributor

@Iniquitatis while we haven't entirely reached a consensus on what a future solution may look like, we took long time to discuss this in rocket chat and rotating the imported models cannot be done at this stage of Godot4, because it requires changes to everything (meshes, animations, retargeting). If it's made by default it will break everything made so far with Godot4 (we're at stable phase) and if we make it optional everything has to support the multiple option and it's going to make the code way too complicated. Changing look_at and reverting the front view in editor is the only compromise that's functional at this point in time.

@aaronfranke
Copy link
Member

aaronfranke commented Apr 28, 2023

@Iniquitatis See this PR I opened #72753. This adds an import setting to rotate by 180 degrees. It is completely optional, off by default, so it does not break compatibility. It also does not conflict with this PR so we can have both. It does not require changes to everything and it does not make the code way too complicated, it's a small amount of code.

TokageItLab pushed a commit to reduz/godot that referenced this pull request May 24, 2023
This is a much simpler attempt to solve the same problem as godotengine#76060, but without breaking any compatibility.

* Adds a description of what model space is in the Vector3 enums (MODEL_* constants). This has the proper axes laid out for imported 3D assets.
* Adds the option to `look_at` using model_space, which uses Vector3.MODEL_FRONT as forward vector.

The attempt of this PR is to still break the assumption that there is a single direction of forward (which is not the case in Godot)
and make it easier to understand where 3D models are facing, as well as orienting them via look_at.
@TokageItLab
Copy link
Member

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.