Cleanup and simplify camera override API #52285
Conversation
|
Oh, actually, #49334 won't be fixed since that's a 3.3.2 bug. Nonetheless, the same bug is present in master which this will fix and I'm happy to backport this once it's approved and merged. |
scene/main/viewport.h
Outdated
| #ifdef TOOLS_ENABLED | ||
| struct Camera2DOverride { | ||
| bool enabled = false; | ||
| ObjectID previous_camera = ObjectID(); |
There was a problem hiding this comment.
ObjectID is default initialized so you don't need to assign a new one here (same below).
There was a problem hiding this comment.
Ah, good spot! Thanks, fixed now. 🙂
rxlecky
force-pushed
the
camera-override-cleanup
branch
from
380c128 to
06ed733
Compare
|
@akien-mga unless there are further comments on code cleanliness, this should be good to be merged, I'm just finishing the merge conflict resolution, it looks rather trivial. But I'd still wait before merging. I'm not entirely sure whether it was @reduz 's suggestion back when this was first implemented to not use nodes or it was my initiative. Also, besides switching to nodes, this PR also switches from the regular It would be good to get his - or other experienced member's, maybe @Calinou since this is partially a UX change as well - opinion on these aspects before merging. |
- Make the camera override 2D and 3D APIs use the same terminology - Make both 2D and 3D override use Camera nodes under the hood to make the code simpler and cleaner - Make camera override 2D use global canvas transform to make the override camera behave identically to the 2D editor camera, i.e. make CanvasLayers move with the camera regardless of their follow_viewport property value - Makes all code depending on the viewport current camera, i.e. VisibleOnScreenNotifiers, work with the camera override out of the box
rxlecky
force-pushed
the
camera-override-cleanup
branch
from
06ed733 to
cf7a6b9
Compare
|
@akien-mga do you think this could get reviewed sometime soon? I'm happy to merge and rebase but I'm putting it off for now until someone has the time for review. |
akien-mga
requested review from
a team and
RandomShaper
and removed request for
a team
|
Camera overrides were replaced by the Game tab: #97257 |
The change to using camera nodes also has potential downsides. If there's gameplay code depending on current camera, it might cause unexpected behaviour when the current camera suddenly changes to the overriden camera. I had a person ask me whether using the camera override will make the environment unload if they are using visibility culling, causing, for example, the player character to fall through the floor. Similar arguments were the reason, why I originally opted for only creating camera on the RenderingServer without creating the actual camera node. That way, everything would work as if the override camera never existed.
However, reevaluating that now, duplicate code had to be written to support the same functions that the camera nodes already support. Additionally, there are cases where we do want the override camera to work as normal camera, example would be the case described in the issue #49334. All these would have to be catered to separately if the camera override didn't use the camera nodes, whereas now they all "just work". Lastly, to follow up on the visibility culling argument, I think the camera override wouldn't be really that useful if nothing besides the area in player's close vicinity would be loaded and rendered anyway.
Any feedback is welcome. If there's another approach that I haven't thought of, I'm happy to rework this. Cheers!
Closes #49334