blender/blender
165
518
Fork 791
Code Issues 6.8k Pull Requests 942 Projects 21 Wiki Activity

Geometry Nodes: Add Active Camera input node #113431

Merged
Hans Goudey merged 3 commits from Douglas-Paul/blender:active-camera-node into main 2023-12-12 19:11:15 +01:00
Conversation 8 Commits 3 Files Changed 11 +88 -7
Douglas Paul commented 2023-10-09 00:03:16 +02:00
Contributor
Copy Link

This adds a new "Active Camera" input geometry node, per #105761.

The node has two sockets:

  • An object socket labeled "Active Camera" that retrieves the scene's
    current active camera
  • A boolean socket labeled "Has Active Camera" that retrieves whether
    or not the scene has an active camera

The node is available from Input > Scene > Active Camera in the
geometry nodes Add menu.

Typical usage would be to connect this node to an Object Info node to
obtain its transform. This works as expected when the camera's
transform is animated, and also when there are markers on the timeline
that change the active camera.

In order to support the aforementioned changes in the active camera,
this implementation adds depsgraph relations for all cameras referenced
by timeline markers. This eliminates the complexity of updating the
depsgraph whenever the scene switches to a different active camera,
but of course it comes at the cost of including more objects than
strictly necessary in the depsgraph for scenes that switch cameras.
Dynamically updating the depsgraph upon camera changes could be a
future improvement if there proves to be sufficient need for it.

Here is a screenshot of the new node:
active_camera_node.png

I have also attached a .blend file that exercises this new node in a few different situations. (It includes a README text block that describes each test case.)

This adds a new "Active Camera" input geometry node, per #105761. The node has two sockets: * An object socket labeled "Active Camera" that retrieves the scene's current active camera * A boolean socket labeled "Has Active Camera" that retrieves whether or not the scene has an active camera The node is available from Input > Scene > Active Camera in the geometry nodes Add menu. Typical usage would be to connect this node to an Object Info node to obtain its transform. This works as expected when the camera's transform is animated, and also when there are markers on the timeline that change the active camera. In order to support the aforementioned changes in the active camera, this implementation adds depsgraph relations for all cameras referenced by timeline markers. This eliminates the complexity of updating the depsgraph whenever the scene switches to a different active camera, but of course it comes at the cost of including more objects than strictly necessary in the depsgraph for scenes that switch cameras. Dynamically updating the depsgraph upon camera changes could be a future improvement if there proves to be sufficient need for it. --- Here is a screenshot of the new node: ![active_camera_node.png](/attachments/ffe62bd3-7d41-47a3-b921-c775c3bb9920) I have also attached a .blend file that exercises this new node in a few different situations. (It includes a README text block that describes each test case.)
active_camera_node.png
32 KiB
105761_active_camera_node_tests.blend
1020 KiB
👍 4 ❤️ 4
Douglas Paul commented 2023-10-09 01:43:01 +02:00
Author
Contributor
Copy Link

I want to acknowledge that there is another (pre-existing) pull request by someone else for this same issue: !106065

I unfortunately overlooked the existence of that pull request until I had already completed my implementation. However, after reviewing the state of that other pull request, I have decided to submit my own anyway because I believe it's in a more "ready-to-merge" state.

  • My implementation takes a different approach to handling changes to the active camera. It's perhaps less optimal for some cases, but it's simpler and lower-risk.
  • The last comment on the other pull request mentions that there's a crash when using markers, and the author does not know how to resolve it. My implementation does not crash when using markers, or in any of the other situations I've tested it in.
  • My implementation includes an additional "Has Active Camera" output port for completeness, and I have included descriptions for both ports.
  • I believe my code aligns better with the naming conventions used in existing nodes.

However, I do feel bad for stepping on an existing pull request.

I want to acknowledge that there is another (pre-existing) pull request by someone else for this same issue: !106065 I unfortunately overlooked the existence of that pull request until I had already completed my implementation. However, after reviewing the state of that other pull request, I have decided to submit my own anyway because I believe it's in a more "ready-to-merge" state. * My implementation takes a different approach to handling changes to the active camera. It's perhaps less optimal for some cases, but it's simpler and lower-risk. * The [last comment on the other pull request](https://projects.blender.org/blender/blender/pulls/106065#issuecomment-999526) mentions that there's a crash when using markers, and the author does not know how to resolve it. My implementation does not crash when using markers, or in any of the other situations I've tested it in. * My implementation includes an additional "Has Active Camera" output port for completeness, and I have included descriptions for both ports. * I believe my code aligns better with the naming conventions used in existing nodes. However, I do feel bad for stepping on an existing pull request.
Iliya Katushenock added this to the Module: Nodes & Physics project 2023-10-09 05:49:52 +02:00
Iliya Katushenock added the
Interest
Dependency Graph
Interest
Geometry Nodes
 labels 2023-10-09 05:50:03 +02:00
Douglas Paul commented 2023-10-09 14:35:12 +02:00
Author
Contributor
Copy Link

A note/question for the code review: I thought I would need an additional case like this in DepsgraphNodeBuilder::build_nodetree() to guarantee that the necessary depsgraph nodes would exist for the camera objects before I later add relations to those nodes:

else if (ELEM(bnode->type, GEO_NODE_INPUT_ACTIVE_CAMERA)) {
  build_scene_camera(scene_);
}

But I found that everything seems to work fine without those lines, so I decided to omit them to avoid referencing a specific node type in that method if it wasn't strictly necessary. But I am very new to the Blender code, so it seems best for someone with more experience to weigh in.

A note/question for the code review: I thought I would need an additional case like this in `DepsgraphNodeBuilder::build_nodetree()` to guarantee that the necessary depsgraph nodes would exist for the camera objects before I later add relations to those nodes: else if (ELEM(bnode->type, GEO_NODE_INPUT_ACTIVE_CAMERA)) { build_scene_camera(scene_); } But I found that everything seems to work fine without those lines, so I decided to omit them to avoid referencing a specific node type in that method if it wasn't strictly necessary. But I am very new to the Blender code, so it seems best for someone with more experience to weigh in.
Iliya Katushenock commented 2023-10-09 14:42:13 +02:00
Member
Copy Link

@Douglas-Paul Did you mean, to ensure what depgraph node for an active camera is exist at depgraph relation building stage?

@Douglas-Paul Did you mean, to ensure what depgraph node for an active camera is exist at depgraph relation building stage?
Douglas Paul commented 2023-10-09 15:28:27 +02:00
Author
Contributor
Copy Link

@mod_moder If I'm understanding your question correctly, then yes. It seems like it's important that something call DepsgraphNodeBuilder::build_object() on the relevant cameras to ensure there are depsgraph nodes for the cameras before I later try to add relations between those cameras and the Geometry Nodes modifier. But it seems like the depsgraph nodes for the cameras always exist by the time I'm adding the relations, even if I don't do anything to explicitly ensure they exist.

It seems like if a camera is referenced by a timeline marker or directly by the scene, then something always ends up calling build_object() on it before I add my relations. So I think it's fine for me to omit the code above that would guarantee the existence of depsgraph nodes for the cameras. I just thought I'd mention it just in case there's a corner case I haven't thought of.

@mod_moder If I'm understanding your question correctly, then yes. It seems like it's important that something call `DepsgraphNodeBuilder::build_object()` on the relevant cameras to ensure there are depsgraph nodes for the cameras before I later try to add relations between those cameras and the Geometry Nodes modifier. But it seems like the depsgraph nodes for the cameras always exist by the time I'm adding the relations, even if I don't do anything to explicitly ensure they exist. It seems like if a camera is referenced by a timeline marker or directly by the scene, then something always ends up calling `build_object()` on it before I add my relations. So I think it's fine for me to omit the code above that would guarantee the existence of depsgraph nodes for the cameras. I just thought I'd mention it just in case there's a corner case I haven't thought of.
👍 1
Hans Goudey requested review from Jacques Lucke 2023-10-10 14:22:58 +02:00
Hans Goudey requested review from Hans Goudey 2023-10-10 14:22:58 +02:00
Douglas Paul commented 2023-10-10 18:06:42 +02:00
Author
Contributor
Copy Link

A few comments came up in #nodes-physics-module when I asked for reviewers a little while ago, and I had to step away before I got to address them there. Probably better to discuss them here anyway.

Hans Goudey: Depending on "all cameras" doesn't seem great, but I do like that it gets us a functional state that can be improved separately

It doesn't necessarily depend on all cameras—only the active camera and any referenced by timeline markers. But yes, it's not optimal, just a simple and low-risk place to start that I expect will be fine for most use cases.

Iliya Katueshenock: But not sure if Has Active Camera is make sence
Hans Goudey: How about Exists?

I included that output because it adds flexibility/completeness without adding any significant complexity. I'm not attached to the idea of keeping it, though.

As for the name, I'm not opposed to changing it to "Exists". I thought "Has Active Camera" felt a little more appropriate, and there is precedent for "Has"-style names ("Has Alpha" in Image Info and "Has Neighbor" in Index of Nearest), but I don't feel strongly about it.

Another thought that crossed my mind on the boolean output: A more flexible/powerful alternative would be an "Exists" node that checks whether an object exists or not (i.e. is it a null object reference). Or even more generally, a node that compares two object inputs and outputs whether they refer to the same object or not (and leaving one input empty would allow checking for null/empty object). I'm not sure there's sufficient motivation for such a node right now, but maybe worth considering eventually.

A few comments came up in #nodes-physics-module when I asked for reviewers a little while ago, and I had to step away before I got to address them there. Probably better to discuss them here anyway. > **Hans Goudey**: Depending on "all cameras" doesn't seem great, but I do like that it gets us a functional state that can be improved separately It doesn't necessarily depend on _all_ cameras—only the active camera and any referenced by timeline markers. But yes, it's not optimal, just a simple and low-risk place to start that I expect will be fine for most use cases. > **Iliya Katueshenock**: But not sure if `Has Active Camera` is make sence > **Hans Goudey**: How about `Exists`? I included that output because it adds flexibility/completeness without adding any significant complexity. I'm not attached to the idea of keeping it, though. As for the name, I'm not opposed to changing it to "Exists". I thought "Has Active Camera" felt a little more appropriate, and there is precedent for "Has"-style names ("Has Alpha" in Image Info and "Has Neighbor" in Index of Nearest), but I don't feel strongly about it. Another thought that crossed my mind on the boolean output: A more flexible/powerful alternative would be an "Exists" _node_ that checks whether an object exists or not (i.e. is it a null object reference). Or even more generally, a node that compares two object inputs and outputs whether they refer to the same object or not (and leaving one input empty would allow checking for null/empty object). I'm not sure there's sufficient motivation for such a node right now, but maybe worth considering eventually.
👍 1 ❤️ 2
Douglas Paul force-pushed active-camera-node from ea38cd2fd7 to b6440ae992 2023-10-21 01:48:19 +02:00 Compare
Hans Goudey requested changes 2023-11-03 10:32:10 +01:00
Hans Goudey left a comment
Member
Copy Link

Thanks for the PR, and sorry for the delayed response!

I went over this with Sergey and our thought was that this should use a new depsgraph node (blender::deg::NodeType) called something like SCENE_ACTIVE_CAMERA. The point of that is abstracting the behavior and reducing the number of relations.

Sergey also made #114443 after looking at this, that's a bit related.

Besides that, Dalai and I weren't sure about the "Has Active Camera" output. We thought there might be other ways to detect that ("empty object" node like you mentioned above or something) later on, and that it would be best to leave it out for now.

Thanks for the PR, and sorry for the delayed response! I went over this with Sergey and our thought was that this should use a new depsgraph node (`blender::deg::NodeType`) called something like `SCENE_ACTIVE_CAMERA`. The point of that is abstracting the behavior and reducing the number of relations. Sergey also made #114443 after looking at this, that's a bit related. Besides that, Dalai and I weren't sure about the "Has Active Camera" output. We thought there might be other ways to detect that ("empty object" node like you mentioned above or something) later on, and that it would be best to leave it out for now.
Douglas Paul added 2 commits 2023-12-10 01:47:56 +01:00
Douglas Paul commented 2023-12-10 03:26:42 +01:00
Author
Contributor
Copy Link

Thanks, @HooglyBoogly, now it's my turn to apologize for the delayed response :)

I spent a decent number of hours looking into changing the implementation to instead add a new depsgraph node as you requested, but I'm afraid I have to admit defeat on that one. I couldn't find any examples of a depsgraph node dynamically updating its relations as this node would need to, and I ran out of steam trying to understand the depsgraph well enough to do it from scratch.

So I've removed the "Has Active Camera" output as requested and merged in main to resolve conflicts, but I don't plan on making any significant changes to the implementation in this branch. I will leave it to you to decide whether to merge this as-is or hold out for a more optimal implementation.

I would just suggest that if you're looking for a more optimal/advanced solution, you might want to to remove the "Good First Issue" label from #105761 and update the guidance there (to mention e.g. the desire for a new depsgraph node).

Thanks, @HooglyBoogly, now it's my turn to apologize for the delayed response :) I spent a decent number of hours looking into changing the implementation to instead add a new depsgraph node as you requested, but I'm afraid I have to admit defeat on that one. I couldn't find any examples of a depsgraph node dynamically updating its relations as this node would need to, and I ran out of steam trying to understand the depsgraph well enough to do it from scratch. So I've removed the "Has Active Camera" output as requested and merged in `main` to resolve conflicts, but I don't plan on making any significant changes to the implementation in this branch. I will leave it to you to decide whether to merge this as-is or hold out for a more optimal implementation. I would just suggest that if you're looking for a more optimal/advanced solution, you might want to to remove the "Good First Issue" label from #105761 and update the guidance there (to mention e.g. the desire for a new depsgraph node).
Cartesian Caramel commented 2023-12-11 22:56:24 +01:00
First-time contributor
Copy Link

Just to add some feedback:

The current workaround to get the active camera info is to use many drivers with view layer attributes:
Active Camera 1.png Active Camera 2.png

With the object info node, it might be limited to location, rotation, and scale:
Camera info.png

A possible generalized approach could be to have an object/view layer attribute node that would be able to access the data that the drivers have access to. This data can be found in the python tooltips. Mockup/proposal:
Camera Attribute Proposal.png

It's a bit out of the scope for this task, but it may be useful in the future.

Just to add some feedback: The current workaround to get the active camera info is to use many drivers with view layer attributes: ![Active Camera 1.png](/attachments/1b9051ab-5eff-4073-9376-708b590ce0f2) ![Active Camera 2.png](/attachments/57fd46a2-f37a-4bb4-bec1-a3382f4ac55a) With the object info node, it might be limited to location, rotation, and scale: ![Camera info.png](/attachments/ea2e0b24-8ed0-4bf6-a582-7e73184ea8d5) A possible generalized approach could be to have an object/view layer attribute node that would be able to access the data that the drivers have access to. This data can be found in the python tooltips. Mockup/proposal: ![Camera Attribute Proposal.png](/attachments/239b62f6-95b8-44f2-9e4c-332b64b310d1) It's a bit out of the scope for this task, but it may be useful in the future.
Camera info.png
82 KiB
Active Camera 2.png
37 KiB
Active Camera 1.png
168 KiB
Camera Attribute Proposal.png
51 KiB
👍 3 👎 1
Hans Goudey approved these changes 2023-12-12 19:05:21 +01:00
Hans Goudey left a comment
Member
Copy Link

We talked about this in the latest module meeting. We decided it was reasonable to commit this with the current approach to the depsgraph.

We talked about this in the latest [module meeting](https://devtalk.blender.org/t/2023-12-12-nodes-physics-module-meeting/32506). We decided it was reasonable to commit this with the current approach to the depsgraph.
🎉 1
Hans Goudey merged commit 75f160ee96 into main 2023-12-12 19:11:15 +01:00
Douglas Paul commented 2023-12-12 20:22:47 +01:00
Author
Contributor
Copy Link

Thanks, @HooglyBoogly. As a natural follow-up to this, I'll create a Design issue tonight for a Camera Info node to expose camera-specific info.

I'll also make sure there's a task for eventually replacing this simple implementation with a more optimal one based on a new depsgraph node. (I don't plan on working on that task, but I'd love to see someone else do it—especially because I think I could learn a lot from seeing how it was implemented.)

Thanks, @HooglyBoogly. As a natural follow-up to this, I'll create a Design issue tonight for a Camera Info node to expose camera-specific info. I'll also make sure there's a task for eventually replacing this simple implementation with a more optimal one based on a new depsgraph node. (I don't plan on working on that task, but I'd love to see someone else do it—especially because I think I could learn a lot from seeing how it was implemented.)
❤️ 4
Sign in to join this conversation.
Reviewers
No reviewers
Jacques Lucke
Hans Goudey
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset System
Asset datablocks, libraries, browser and shelf

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Code Documentation
Code comments, Python/RNA API descriptions

  
Interest
Collada

  
Interest
Compatibility
Backward and forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Viewport & EEVEE

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Wayland windowing on Unix

  
Interest
Workbench

  
Interest: X11

Xorg/X11 windowing

  
Legacy
Asset Browser Project

Archived

  
Legacy
Blender 2.8 Project

Archived

  
Legacy
Milestone 1: Basic, Local Asset Browser

Archived

  
Legacy
OpenGL Error

Archived

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Related to security, see policy: https://developer.blender.org/docs/handbook/bug_reports/vulnerability_reports/

  
Module
Animation & Rigging

  
Module
Asset System

  
Module
Core

  
Module
Development Management

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Module
Viewport & EEVEE

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Severity
High

  
Severity
Low

  
Severity
Normal

  
Severity
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

Archived

  
Type
Report

Archived

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Asset System
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Module: Nodes & Physics
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#113431
No description provided.
Delete Branch "Douglas-Paul/blender:active-camera-node"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?