-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Conversation
There was a problem hiding this 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.
@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. |
There was a problem hiding this 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).
abf87e8
to
1e3c28b
Compare
834bf5b
to
47378c9
Compare
3f68d49
to
cacd543
Compare
cacd543
to
7f100a0
Compare
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.
7f100a0
to
8de088d
Compare
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.
The ideal solution is to convert the coordinate system when importing. i.e. instead of simply rotating the model 180 degrees, vector So we decided to keep the imported model as-is last night, and:
I think we must admit that this is a workaround for a problem that we can't afford to fix correctly. |
@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). |
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. |
@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]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vector3.FRONT
@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. |
@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. |
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.
Superseded by #76082. |
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.
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
isVector3::CAMERA_FORWARD
(pointing to Z-, which confused many users). And aVector3::FRONT
is added that is Z+.This PR also requires #76052 being merged to ensure everything is fully consistent again.