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

Why node.camera instead of camera.node? #836

Closed
javagl opened this issue Feb 11, 2017 · 15 comments
Closed

Why node.camera instead of camera.node? #836

javagl opened this issue Feb 11, 2017 · 15 comments

Comments

@javagl
Copy link
Contributor

javagl commented Feb 11, 2017

This may look like a very basic question, and it feels odd that I can't find an answer, but why are cameras attached to nodes, in contrast to defining a node that each camera is attached to?

Particularly, with the current pattern of the node containing a reference to a camera, it would be possible to attach the same camera to multiple nodes. The spec does not say anything about this. The validator does not seem to check this either. What should happen in this case?
(This would not be possible with a camera.node property).

@lexaknyazev
Copy link
Member

As far as I understand, camera object is just a storage of optic parameters. Real instantiation of camera occurs with node.camera property.

@javagl
Copy link
Contributor Author

javagl commented Feb 11, 2017

On the one hand, it's nice to have an entity that represents a camera, and that has no dependencies to its environment (i.e. no dependencies to a node). But I thought that, from a modeling point of view, it would be cleaner and less ambiguous to have the dependency in the other direction.

I thought that I might be overlooking a reason for the current solution. Of course, for things like meshes, the idea of having one 'instance' attached to each node makes sense, but for the cameras, this seemed a bit odd...

So considering the case of one camera being attached to multiple nodes...

"nodes" : {
    "node0" : { "camera" : "camera0" },
    "node1" : { "camera" : "camera0" },
},
"cameras": {  "camera0": { "name" : "MyCamera" ... } },

the surrounding application will have to differentiate them accordingly, as MyCamera.node0 and MyCamera.node1...

@lexaknyazev
Copy link
Member

the surrounding application will have to differentiate them accordingly, as MyCamera.node0 and MyCamera.node1...

From app perspective, these are two different cameras, since they are likely located in different places. One could argue that camera should be called lens.

Such approach (camera attached to node) is quite common. Compare:

@javagl
Copy link
Contributor Author

javagl commented Feb 11, 2017

The linked documentations also don't seem to say anything about the case of one camera being referenced multiple times, but from a quick glance at the COLLADA spec, there seem to be <camera> and <instance_camera>, which may correspond to the idea of "instantiating" a camera (although I didn't read it thoroughly)

The core point is that a camera in glTF is not an "entity" that is attached to a node. So a camera is rather a "template", for the actual camera object that has to be instantiated each time that it is referenced in a node. This was not entirely clear or obvious for me from a modeling perspective.

(The background: In my viewer, I bascially allowed selecting a camera object (via its ID), using a dropdown menu. This required some odd quirks, involving a mapping cameraID -> nodeID to find the node that refers to the camera and provides the transform. This, of course, does not make sense for the case where one camera is attached to two nodes, and will be refactored - possibly ending up with something like a CameraInstance class...)

@recp
Copy link

recp commented Feb 12, 2017

Recently I've made my stuff public, after seen this issue I've implemented camera list, because I also want to show all cameras to user like you.

https://github.com/recp/assetkit/blob/master/include/assetkit.h#L904

ak_instanceName function gives a name for camera instance, instance camera may not have a name so it generates a name using ID and given index.

On COLLADA side, selecting cameras is quite easy, multiple nodes can instantiate a camera by <instance_camera>, we can collect them during parsing <node> element .

(The background: In my viewer, I bascially allowed selecting a camera object (via its ID), using a dropdown menu. This required some odd quirks, involving a mapping cameraID -> nodeID to find the node that refers to the camera and provides the transform. This, of course, does not make sense for the case where one camera is attached to two nodes, and will be refactored - possibly ending up with something like a CameraInstance class...)

Maybe this can be done on glTF too? I've created a list and add instances to list when parsing nodes, when reading names (ak_instanceName), I'm using <instance_camera>'s name or generated name like CameraID1-1, CameraID1-2 NOT ID because ID is unique and camera can be used many times? I think this way is reasonable

Also Node.Camera makes sense because consider a human; human eye[s] is camera and camera moves with human-node[s], parent is human not camera

@javagl
Copy link
Contributor Author

javagl commented Feb 12, 2017

@recp Yes, I could imagine that other loaders and viewers also get this wrong.

(I tried to have a look at some of them, but ... sigh.. /* comments */ seem to be considered as old-fashioned or whatever...).

Maybe the spec/README can be a bit more explicit about this point: A camera is not an "entity". I'll consider a PR for this.

@javagl
Copy link
Contributor Author

javagl commented Feb 12, 2017

Related to the discussion about cameras in #815 (comment) (where it was rather off-topic), the question about the "default camera" seems to be more subtle now.

Regardless of which strategy a viewer uses to select its "initial, default camera": A reordering of the nodes in the JSON file may change this "initial, default camera". But maybe this is not considered as an important point.

@javagl
Copy link
Contributor Author

javagl commented Feb 12, 2017

Just dumping further questions here:

When a viewer can load multiple glTF assets at the same time, should it be possible to select the camera for each asset individually? Alternatively, an asset can (or has to) be rendered with the "camera instance" of a "foreign" asset...

@lexaknyazev
Copy link
Member

Regardless of which strategy a viewer uses to select its "initial, default camera": A reordering of the nodes in the JSON file may change this "initial, default camera".

I think at some point we could add a "hint" to default (per-asset or even per-scene) camera node as with scene top-level property. @pjcozzi

should it be possible to select the camera for each asset individually

I don't see any issue here:

  • camera object is just a container for optics (i.e. projection matrix). Nothing to do with scenes/assets.
  • Actual camera is being instantiated by node, which has clear affiliation with scene-graph of a particular asset.
  • Mixing multiple glTF assets in one context has much more implications than just camera selection, so such behavior is definitely out of scope.

@javagl
Copy link
Contributor Author

javagl commented Feb 12, 2017

Mixing multiple glTF assets in one context has much more implications than just camera selection, so such behavior is definitely out of scope.

That depends on what the "context" is in this case. I don't see how something like

viewer.add(loadModel("duck.gltf"));
viewer.add(loadModel("duck.gltf"));
viewer.add(loadModel("goose.gltf"));

should raise any issues, except for the parts of the state that are maintained by the surrounding application. Each asset has a local "context". And (from the tip of my head), the only part that leaks into the "global (application) context" is the "active camera".

(Maybe also the time for animations - roughly: Whether the input time for all these models should be the same, or whether they can all have indidual "animation time offsets". But that's another point, unrelated to this one.

However, I'm currently thinking about letting the viewer provide a list of "CameraModels" (which are the instantiations of all cameras of all glTF assets that are shown in the viewer), and allowing to set the CameraModels for each asset individually, or using the same CameraModel for all assets, but am not sure whether this is the final solution.

@lexaknyazev
Copy link
Member

the only part that leaks into the "global (application) context" is the "active camera".

Each asset could have its own set of scenes. How should they be mixed?
Should nodes, that don't belong to any scene, be loaded into the same "app" context?
Lights could raise the same questions as camera instances.


In the simplest case, one could load everything into one app context and treat all nodes as if they came from one source.

@javagl
Copy link
Contributor Author

javagl commented Feb 12, 2017

For lights, I think that the scope is clearly defined: A light from one asset should not (and cannot, sensibly) shine on the geometry of another asset.

The set of scenes is defined locally, for each asset. Similarly to the option to select the "active" scene for one asset, one could offer the option to select the active scene for each asset.

The difference for the cameras is that the "set of all camera instances" has to be maintained in the application, and the individual assets are not aware of it - and eventually, an asset might be rendered with a camera that it does not contain.

(Note: All these points are comparatively minor ones. The skinning and texture formats are far more important regarding the spec (although I'm afraid I cannot say anything profound there). I was just wondering about these points, and whether there was an "intended" behavior for the applications, based on the current design)

@pjcozzi
Copy link
Member

pjcozzi commented Feb 13, 2017

I think at some point we could add a "hint" to default (per-asset or even per-scene) camera node as with scene top-level property. @pjcozzi

Agreed, as long as the app would be allowed to ignore it as we discussed elsewhere.

@pjcozzi
Copy link
Member

pjcozzi commented Feb 13, 2017

@recp assetkit looks great. When ready, please open a pull request to add it to the README.md in this repo.

@donmccurdy
Copy link
Contributor

This appears resolved to me, but feel free to reopen if there's more to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants