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 Camera3D has an incorrect shape before opening the 2D tab #70628

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ayush-singla27
Copy link
Contributor

@Ayush-singla27 Ayush-singla27 commented Dec 27, 2022

Fixed default shape of frustum of camera3D, which had incorrect shape before opening the 2D tab.

Production edit: Fixes #70362

@Ayush-singla27 Ayush-singla27 requested a review from a team as a code owner December 27, 2022 12:50
@DmitriySalnikov
Copy link
Contributor

This is what the behavior of frustum should look like, it seems to me (Godot 3.x):
Godot_v3 5 1-stable_win64_lV18GvEzX3

But while I was testing this PR, I found a more detailed pattern. Frustum doesn't change the first time you switch to a 2D tab, it changes every time you switch to a 2D tab. (But I didn't notice this when I created the issue 😞 )
(recorded with v4.0.beta.custom_build [1ea6a5d38])

GxFe8hqglN.2.mp4

Also, the size of the Camera3D gizmo itself in godot 4.x changes as correctly as in godot 3.x
godot windows editor x86_64_JVsBtVWw3p

It turns out that this PR does not solve the problem yet.

@Ayush-singla27
Copy link
Contributor Author

@DmitriySalnikov so the frustum should be independent on viewport size or the frustum should scale without switching to 2D tab?

@DmitriySalnikov
Copy link
Contributor

DmitriySalnikov commented Dec 28, 2022

I do not know how it worked inside before. But before godot 4, changing the window size parameters always affected Camera3D(gizmo and get_frustum()) on the 3D tab and the size of this area on the 2D tab:
image
The size of this rectangle was also changed
image
Current Camera frustum is great for preview mode, but it still only updates when switching to the 2D tab...

By the way, Camera3D inside the SubViewport works correctly, as in godot 3.
godot windows editor x86_64_gf31kyEu8O
godot windows editor x86_64_IUTdsp39da

@Ayush-singla27
Copy link
Contributor Author

@DmitriySalnikov should changing viewport size (in Project setting) affect SubViewport camera frustum ?

@DmitriySalnikov
Copy link
Contributor

@DmitriySalnikov should changing viewport size (in Project setting) affect SubViewport camera frustum ?

No I think not. Even if you look at Camera3D gizmo, you can see that the shape of the camera inside the SubViewport depends only on the size of this subviewport. And a camera without a subviewport changes shape based on the size from ProjectSettings.

godot.windows.editor.x86_64_hEUvFbv4w9.mp4

@Ayush-singla27
Copy link
Contributor Author

@DmitriySalnikov i think now it should work like godot 3. :D

@DmitriySalnikov
Copy link
Contributor

This works well, but only as long as the SubViewport has the name SubViewport...

@Ayush-singla27
Copy link
Contributor Author

@DmitriySalnikov i am have some trouble solving this. The following code

        bool check = false;
	Node *temp = this->get_node(this->get_path());
	if (temp != NULL) {
		if (temp->get_class_name() == "SubViewport") {
			check = true;
			print_line(temp->get_class_name());
		}
		else {
			check = false;
		}
		temp = temp->get_parent();
			
	}

is giving the following output for demo project attached in this issue

subviewport

but the scene tree does not have any subViewport node.

@DmitriySalnikov
Copy link
Contributor

I don't know a best way to find out which SubViewport is the root for the current scene

@Ayush-singla27
Copy link
Contributor Author

@DmitriySalnikov finally! i think this should do it. ✌️

scene/main/viewport.h Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@DmitriySalnikov DmitriySalnikov left a comment

Choose a reason for hiding this comment

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

After making changes, you will need to squash commits so that only 1 commit remains in the end. This is required by the core team.
https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase
If everything is messed up, you will have to restore the branch from GitHub or a local backup.

scene/main/viewport.cpp Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@DmitriySalnikov DmitriySalnikov left a comment

Choose a reason for hiding this comment

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

Everything works as intended!

However, I can't imagine why there was a condition size == Size2(), because if the size is less than 2px, then the engine throws a lot of errors and sometimes crashes (v4.0.beta10.official [d0398f6]). So I think there should be no problems.

@akien-mga akien-mga changed the title Fixed: Camera3D has an incorrect shape before opening the 2D tab Fix Camera3D has an incorrect shape before opening the 2D tab Feb 17, 2023
@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023
@DmitriySalnikov
Copy link
Contributor

@akien-mga godot 4.0 has already been released. Is it possible to merge this PR now?

@akien-mga
Copy link
Member

Not yet, I'm not sure this fix is appropriate. Fetching project settings all the time for such a core method doesn't sound like a good option. The real issue might be deeper and this looks like a workaround.

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
@YuriSizov YuriSizov removed this from the 4.2 milestone Oct 27, 2023
@YuriSizov YuriSizov added this to the 4.x milestone Oct 27, 2023
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.

Frustum of Camera3D does not take into account the window size from ProjectSettings
4 participants