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.

Sign up for GitHub

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

Jump to bottom

add viewport.get_camera_2d() #38317

Merged
merged 1 commit into from
Jul 20, 2021
Merged

add viewport.get_camera_2d() #38317

merged 1 commit into from
Jul 20, 2021

Conversation

verdog
Copy link
Contributor

@verdog verdog commented Apr 29, 2020

Fixes #5411
First time contributor, still learning how things fit together, so I'd be happy to make any adjustments needed. 🙂

The viewport.get_camera() functionality already exists in 3d, so I just mimicked how 3d keeps track of things and added it to 2d. I added a Camera2D pointer in the viewport class and it gets updated it as 2d cameras are set as current/turned off.

Unlike 3d, in 2d, it is not required for a camera to be current. If this is the case, get_camera_2d() just returns null (just like what calling get_camera() in 2d does).

@verdog verdog requested a review from a team as a code owner April 29, 2020 00:27
@Chaosus Chaosus added this to the 4.0 milestone Apr 29, 2020
@NuclearPhoenixx
Copy link

Any progress with this PR?

@verdog
Copy link
Contributor Author

verdog commented Jan 10, 2021

I assume that by now another rebase will be needed to merge. Will happily do so if a review passes!

Calinou
Calinou approved these changes Jan 10, 2021
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this can be rebased so it can be tested on latest master first. The feature seems desired enough and was approved by reduz in the original issue, so it's probably good to merge if the code implementation is OK.

@verdog
Copy link
Contributor Author

verdog commented Jan 12, 2021

All fixed up. Just had to updated a few new instances of get_camera and a mention of it in the documentation.

@NuclearPhoenixx
Copy link

Ping

@akien-mga akien-mga requested review from a team and reduz January 18, 2021 11:32
KoBeWi
KoBeWi requested changes Jan 19, 2021
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I checked the code and it looks fine, except one thing. set_current(false) should check if this camera is really current, otherwise it will remove any current camera.

You can test this by creating two cameras, setting first to current and calling set_current(false) on the second one. get_camera_2d() will return null after this operation, while it should return the first camera.

@verdog
Copy link
Contributor Author

verdog commented Jan 28, 2021

Good catch! Fixed that.

@KoBeWi KoBeWi mentioned this pull request Mar 3, 2021
Closed
@Jummit Jummit mentioned this pull request Apr 17, 2021
Open
@ek68794998
Copy link
Contributor

What is the status of this PR? It's been idle for a few months now but seems to be approved.

@Calinou
Copy link
Member

Calinou commented Jun 7, 2021 •
edited
Loading

What is the status of this PR? It's been idle for a few months now but seems to be approved.

This pull request needs to be rebased against the latest master branch before it can merged.

cc @verdog

@verdog verdog requested review from a team as code owners July 3, 2021 18:34
verdog
verdog commented Jul 3, 2021
View reviewed changes
scene/main/viewport.cpp Outdated Show resolved Hide resolved
@verdog
add viewport.get_camera_2d()
879f84d 
* there is now a more clear distinction between camera_2d and camera_3d functions in the engine code
* simplified camera2d's exported interface - now everything happens directly with the 'current' variable and make_current and clear_current are no longer exposed- there were some situations where calling one instead of set_current would result in incomplete results
* rebased to current godot master
@timvisee
Copy link

timvisee commented Jul 20, 2021 •
edited
Loading

Note that this is a breaking change, because it renames Viewport.get_camera() to Viewport.get_camera_3d(). I'm not sure whether that is desired.

@akien-mga
Copy link
Member

Note that this is a breaking change, because it renames Viewport.get_camera() to Viewport.get_camera_3d(). I'm not sure whether that is desired.

Since this targets 4.0, this is a good change, as it matches the rename of Camera to Camera3D (along with many other 3D nodes).

@timvisee
Copy link

(...)

Since this targets 4.0, this is a good change, as it matches the rename of Camera to Camera3D (along with many other 3D nodes).

New to Godot. Thanks for clearing that up!

@akien-mga akien-mga merged commit c82daae into godotengine:master Jul 20, 2021
@akien-mga
Copy link
Member

Thanks! And sorry for the delay reviewing and merging this PR :)

@biblical-text
Copy link

biblical-text commented Mar 10, 2022 •
edited
Loading

Is there a way to get_camera_2d() pre Godot 4? I kind of need to use this now. (I need to get the current camera position, so i can apply the current camera position to the "about to become active" camera position)

@Calinou
Copy link
Member

Calinou commented Mar 10, 2022 •
edited
Loading

Is there a way to get_camera_2d() pre Godot 4? I kind of need to use this now. (I need to get the current camera position, so i can apply the current camera position to the "about to become active" camera position)

For this feature to be available in Godot 3.5, this pull request needs to be remade against the 3.x branch first.

@Deozaan Deozaan mentioned this pull request Dec 1, 2022
Open
@lawnjelly lawnjelly mentioned this pull request Jan 7, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KoBeWi KoBeWi KoBeWi approved these changes

@reduz reduz reduz approved these changes

@Calinou Calinou Calinou approved these changes

Assignees
No one assigned
Labels
enhancement topic:core
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

No 2D equivalent of viewport.get_camera()
10 participants
@verdog @NuclearPhoenixx @ek68794998 @Calinou @timvisee @akien-mga @biblical-text @KoBeWi @reduz @Chaosus