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 3D Viewport Front/Rear axis and Focus button #76052

Merged
merged 2 commits into from
May 24, 2023

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Apr 14, 2023

Supersedes #75921.

We've discussed this with reduz in contributors chat. Revert the commit #45669 that caused it.

There will be additional forward and backward documentation by reduz later.


Includes a fix for an issue that broke the drawing of the Focus button in the upper right corner when in an Othro view parallel to the axis. Also front and rear button index was wrong, fixed.

image

@TokageItLab TokageItLab added bug topic:editor cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:3d labels Apr 14, 2023
@TokageItLab TokageItLab requested a review from reduz April 14, 2023 09:25
@TokageItLab TokageItLab requested a review from a team as a code owner April 14, 2023 09:25
@YuriSizov YuriSizov added this to the 4.x milestone Apr 14, 2023
Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

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

Regardless on the discussion about look_at being confusing, this PR makes Godot work consistently with every other 3D software (CAD, 3D Modelling or game engine), so the front/back views look at the right side of the model. Currently they are flipped.

@capnm
Copy link
Contributor

capnm commented Apr 14, 2023

  • Now
extends Camera3D

@onready var su := $"../Suzanne"

func _ready():
	su.look_at(position)
	su.rotate_object_local(Vector3.UP, PI)

func _process(delta):
	su.rotate_object_local(Vector3.UP, delta)
extends Node3D

func _ready():
	$"../Camera3D".look_at(position)
extends Camera3D

@onready var su := $"../Suzanne"

func _ready():
	su.model_look_at(position)

var wait := 1.0

func _process(delta):
	if wait < 0:
		su.rotate_object_local(Vector3.UP, delta)
	else:
		wait -= delta
extends Node3D

func _ready():
	$"../Camera3D".camera_look_at(position)

suzanne_pr.zip

@lyuma
Copy link
Contributor

lyuma commented Apr 14, 2023

@capnm I think it might make sense to put that comment on the PR you are testing, namely #76060 - it's not directly related to this change.

This change only affects the user interface / the Viewport menu items.

@aaronfranke
Copy link
Member

aaronfranke commented Apr 14, 2023

I just checked what Unity and Unreal do.

  • Unity defines +Z as the forward direction with +X right. The camera faces +Z. "Front" in Unity views the +Z side of the object (view pointing -Z), and "Back" in Unity views the -Z side of the object (view pointing +Z). "Right" in Unity views the +X side of the object (view pointing -X), and "Left" in Unity views the -X side of the object (view pointing +X).

  • Unreal defines +X as the forward direction with +Y right. The camera faces +X. "Front" in Unreal views the +X side of the object (view pointing -X), and "Back" in Unreal views the -X side of the object (view pointing +X). "Right" in Unreal views the +Y side of the object (view pointing -Y), and "Left" in Unreal views the -Y side of the object (view pointing +Y).

Unity vs current Godot:

front-unity

Unity and Unreal both have a consistent forward direction, unlike Blender which does not have a consistent forward direction. Godot in the current master is self-consistent and consistent with how Unity and Unreal work but inconsistent with Blender. This PR would change it to be consistent with how Blender works but not self-consistent and inconsistent with how Unity and Unreal work. By self-consistent I mean that front/right/etc always refer to the same relative directions.

EDIT: To respond to reduz below saying Godot is swapped respect to "any other software"... what about Unity and Unreal, which the current Godot is consistent with, and all are consistent with themselves?

@reduz
Copy link
Member

reduz commented Apr 14, 2023

This needs to be merged because Godot is plain broken. Front/Back are swapped respect to any other software

@aaronfranke I understand your wish to have consistency in this aspect, but having something that mimics Blender is better in practice for us and our users, even if Blender is not consistent. The reasoning in Blender is very smart, its what you would get if you rotate the object in the desired direction and its more on the intuitive than in the consistent side. I much prefer that.

@fracteed
Copy link

@aaronfranke I am going to repost this set of comparison images that I posted in the closed PR. Every 3d dcc on the planet adheres to the same relative top/front/right/left conventions. It is not relevant which coordinate system they use, it is about the artistic expectation of the front and left/right views as compared to the top view.

These are screenshots I took from Blender, Houdini and Cinema4D. Even though they all have different coordinate systems, the ortho views are relatively the same. Cinema4D actually does have the same coordinate system as Godot. The small red cube is in front of the large cube when seen from the top view.

blender_views

houdini_views

c4d_views

This is what it looks like in Godot with the reverted PR, and can be seen to be consistent.
godot_view_)zforward

This is what it currently looks like in Godot 3 and 4:
godot_view_current

@TokageItLab
Copy link
Member Author

@fracteed The last picture is incorrect, I guess the actual current Godot 4 has its left and right reversed (I also fixed that in #75921). And that meant that Godot which is supposed to be a right-handed coordinate system, suddenly has a left-handed coordinate system.

In any case, this PR will revert the view to what it should be.

@fracteed
Copy link

@TokageItLab yes, the last picture is the incorrect one, and is from a current 4.0 build. The previous image (that lines up with all the others) was from a github autobuild that I tried previously with your PR. So, has it changed since yesterday?

@TokageItLab
Copy link
Member Author

TokageItLab commented Apr 15, 2023

@fracteed I meant #75921 to fix the front/back when that option is true, and also to fix the left/right which was like a left-handed coordinate system when that option is false.

This PR simply fixes the front and back only. Now the view will revert to the right hand coordinate system and capture +Z of the model from the front.

@fracteed
Copy link

@TokageItLab I just redownloaded the artifact from this PR to retest. I still get the same correct result. Looks fine to me. Presumably this is what you intended?

views_again

@TokageItLab
Copy link
Member Author

@fracteed Yes, it is equivalent to the PR of #75921 with the option true.

@capnm
Copy link
Contributor

capnm commented Apr 15, 2023

@fracteed Yes, it is equivalent to the PR of #75921 with the option true.

Yes ... but I personally think that such (expert) options in already crowded places will only bring xx k additional bugs and mind-dead-locks to Godot. The mainstream should rather focus on getting the standard open chains like svg, glTF and many others to work without errors. We have unfortunately bugs in every corner here ...

@Zireael07
Copy link
Contributor

I have to agree with @capnm, at the very least the option should be somewhere VERY prominent

@akien-mga akien-mga merged commit 299f0ae into godotengine:master May 24, 2023
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 4.x, 4.1 May 24, 2023
@TokageItLab TokageItLab deleted the fix-viewport-axis branch February 14, 2024 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:editor topic:3d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants