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

feat: camera control components #214

Merged
merged 15 commits into from
Sep 4, 2024
Merged

Conversation

pravusjif
Copy link
Member

@pravusjif pravusjif commented Aug 13, 2024

Introduces the following components:

  • MainCamera: put by the Explorer on initialization on the Camera entity (as it's done with other components like PBCameraMode or PBPointerLock etc.)
  • VirtualCamera

Example usage can be seen in a dedicated test scene using the SDK at: decentraland/sdk7-test-scenes#13

Related PRs:

@pravusjif pravusjif self-assigned this Aug 13, 2024
Copy link

github-actions bot commented Aug 13, 2024

Test this pull request

  • The @dcl/protocol package can be tested in scenes by running
    npm install "https://sdk-team-cdn.decentraland.org/@dcl/protocol/branch//dcl-protocol-1.0.0-10683086755.commit-abd7a07.tgz"

@pravusjif pravusjif requested a review from gonpombo8 September 2, 2024 15:22
@pravusjif pravusjif marked this pull request as ready for review September 2, 2024 15:22
gonpombo8
gonpombo8 previously approved these changes Sep 2, 2024
@robtfm
Copy link
Contributor

robtfm commented Sep 3, 2024

We've developed a similar implementation and noticed some differences in our approaches. Here's a brief comparison of the features:

feature this implementation protocol squad implementation
look at a target supported not currently supported (can be implemented by users)
specify transition supported not currently supported
select by area not currently supported (can be implemented by users) supported
allow user to orient camera (with constraints) not currently supported supported
zoom level not currently supported supported

It seems like we could combine our efforts to create a more robust solution. Going through item by item:

  • The transition feature looks useful, i think we should take it as is.

  • The "look at a target" feature is similar to existing PbBillboard functionality (which also offers fuller control over the axes). It might make sense to extend the PbBillboard with a target entity, or develop a new PbLookAt component with similar functionality. This could provide broader utility beyond just targeting cameras, such as turning NPCs to face the player.

  • For select by area, i think using the existing cameraModeArea infrastructure has some advantages

    • it allows for easier setup if you want different cameras when the player is in different locations
    • it makes clearer the interactions between the "virtual cameras" and cameraModeAreas - how does it work in your implementation if a user enters a force-first-person area while the camera is set to another entity?
  • The user camera controls i think are useful, but could be implemented in unity/alpha later.

  • The zoom level, we use the camera entity's scale.z component to specify a zoom, and the min/max range is for the user to zoom in/out manually as well.

What do you think about potentially merging our efforts to create a more comprehensive solution?

@pravusjif
Copy link
Member Author

pravusjif commented Sep 3, 2024

What do you think about potentially merging our efforts to create a more comprehensive solution?

Hello Rob, so from what I understand if we go with your proposal we would be scratching our implementation completely and basically implementing support for your approach and adding the transition feature only right?

In my opinion the CameraModeArea should only change the camera mode between First or Third person camera. And for more vesatile controls of the camera through the SDK, something separate has to be in place. Because the CameraModeArea limits/binds the camera controls capabilities of the scene to the player position and that doesn't need to be that way... if we can give more freedom to the scene to control the camera without having to have the player in a specific location.

Regarding the point mentioned of "allows for easier setup if you want different cameras when the player is in different locations", that can be done with a trigger area (or player pos checks) to change the camera in specific locations.

The same "easier setup" argument could be said about having the LookAt property in the VirtualCamera: the creator only has to reference an entity in there and the camera will always look at it, as easy as that. However I completely agree on the value of having a new target property in the Billboard component, that could be implemented separately and it's also super useful.

Regarding the interactions between the CameraModeArea and the VirtualCamera+MainCamera components in our approach, the MainCamera always has the priority, the CameraModeArea functionality starts running again when there is no "active virtual camera" (when MainCamera.virtualCameraEntity == 0)

  • The user camera controls i think are useful, but could be implemented in unity/alpha later.

If you mean the demo video, just to clarify it was just manipulating a VirtualCamera entity's transform with the global input, nothing related to the new components.

I do like your approach features of zoom level and allowing user input with constraints (this last one we also though to implement it in the future in the VirtualCamera itself).

I'm all in favor of doing a joint effort on the protocol layer additions/changes, it's just that in this case I believe the VirtualCameras + MainCamera (originally though of as "CameraDirector") components is more versatile for creators (and your approach features we are missing here could be built on top of this in a future iteration).

@pravusjif
Copy link
Member Author

Here is the test scene I've been working with in case you want to take a look at the sdk API and usage for these new components: decentraland/sdk7-test-scenes#13

@robtfm
Copy link
Contributor

robtfm commented Sep 3, 2024

the CameraModeArea limits/binds the camera controls capabilities of the scene to the player position

one can always make an infinite area, then it is exactly the same as using a central component. adding a separate way to control the camera for cinematic cameras only seems confusing to me. cinematic cams and forced 1st/3rd-person cams can't operate at the same time, so why not use the same mechanism for specifying them both?

then, if we started today, it might be better to not use cameraModeArea and use a global component. but given that we have the cameraModeArea already i would say it's better to extend it (or perhaps deprecate it entirely and put all the functionality into a global component).

@gonpombo8
Copy link
Contributor

We've developed a similar implementation and noticed some differences in our approaches. Here's a brief comparison of the features:

feature this implementation protocol squad implementation
look at a target supported not currently supported (can be implemented by users)
specify transition supported not currently supported
select by area not currently supported (can be implemented by users) supported
allow user to orient camera (with constraints) not currently supported supported
zoom level not currently supported supported
It seems like we could combine our efforts to create a more robust solution. Going through item by item:

  • The transition feature looks useful, i think we should take it as is.

  • The "look at a target" feature is similar to existing PbBillboard functionality (which also offers fuller control over the axes). It might make sense to extend the PbBillboard with a target entity, or develop a new PbLookAt component with similar functionality. This could provide broader utility beyond just targeting cameras, such as turning NPCs to face the player.

  • For select by area, i think using the existing cameraModeArea infrastructure has some advantages

    • it allows for easier setup if you want different cameras when the player is in different locations
    • it makes clearer the interactions between the "virtual cameras" and cameraModeAreas - how does it work in your implementation if a user enters a force-first-person area while the camera is set to another entity?
  • The user camera controls i think are useful, but could be implemented in unity/alpha later.

  • The zoom level, we use the camera entity's scale.z component to specify a zoom, and the min/max range is for the user to zoom in/out manually as well.

What do you think about potentially merging our efforts to create a more comprehensive solution?

For content creators having both VirtualCamera & MainCamera is really useful and easy to use. If you see pravus scene-templates is quite intuitive how they work, and they have tons of possibilities. I wont change them for a single PBCameraModeArea.. Whats the main reason on changing the whole approach of this PR for your already-supported implementation ?

For the not supported capabilties, we can re-iterate them in the future, is out of our scope now, and can be added later.

Also in the future if we want to change the lookAt to a PbLookAt component, (because there are more use cases that we want), we can deprecated that property, and start using that component, but using a Billboarcd component is way more complex and for now we don't want those features.

There is no need to do all it now, maybe with this aproach we are good enough, and its all that we need. It doesnt have to be perfect and cover all the cases that we may not use.

@pravusjif
Copy link
Member Author

I'm merging this PR that adds MainCamera and VirtualCamera, since @nearnshaw and @leanmendoza had already talked about the Foundation implementation of the camera controls feature and already agreed that it would be a different implementation than the one previously accomplished by the protocol squad, both implementations can co-exist in the protocol layer and there is no problem with that.

In future iterations we can for sure enrich these components by not only adding the Foundation's planned extensions (camera transition overrides in MainCamera, FOV and similar properties in VirtualVamera) but also plan for adding other properties like the useful ones the protocol squad suggested (zoom, allowing input for rotation, extending the Billboard with a target property).

For next features we'll try to review deeper the potential protocol changes during features design, taking advantage of the weekly meeting between both teams, to try and reach a stronger agreement and also add value as a joint effort from both sides.

@pravusjif pravusjif merged commit a0c6d86 into main Sep 4, 2024
3 checks passed
@pravusjif pravusjif deleted the feat/camera-control-components branch September 4, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants