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

Game camera override #27742

Merged
merged 1 commit into from Nov 8, 2019
Merged

Game camera override #27742

merged 1 commit into from Nov 8, 2019

Conversation

@rxlecky
Copy link
Contributor

rxlecky commented Apr 6, 2019

Added a button to the 2D and 3D editor viewports that toggles replication of the viewport camera into running game.

So far, only 2D camera replication works. 3D is WIP.

@rxlecky rxlecky requested a review from reduz as a code owner Apr 6, 2019
@rxlecky rxlecky changed the title [WIP] Implemented editor viewport camera replication [WIP] Editor viewport camera replication Apr 6, 2019
@rxlecky rxlecky force-pushed the rxlecky:camera-replication branch from 50e77b7 to 474d1de Apr 6, 2019
@Calinou

This comment has been minimized.

Copy link
Member

Calinou commented Apr 6, 2019

This looks great! I was just about to open an issue requesting this feature 🙂

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Apr 6, 2019

Hahah, nice. 😃 At least I know it won't go unused.

This is the current progress in 2D
2D_camrep

@akien-mga akien-mga added this to the 3.2 milestone Apr 7, 2019
@OBKF

This comment has been minimized.

Copy link
Contributor

OBKF commented Apr 7, 2019

Nice, It would be grate to have the game run inside the editor as well (for tweaking values when using sync scene and script changes) if you have a single monitor.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Apr 7, 2019

@OBKF this is already possible. 🙂 When you launch a game, you can make changes to your script while it's running and when you save the script, it is hot-reloaded in a running game. Also changes that you make to values in the inspector are instantly replicated in game.

@groud

This comment has been minimized.

Copy link
Contributor

groud commented Apr 7, 2019

If possible, I think a button in the toolbar would be better.
I know the 3d editor has the preview button displayed on the viewport, but ultimately I think it should be removed in the 3D view, as it kinda clutters the interface.

Else the feature looks cool, nice job. :)

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Apr 7, 2019

@groud I tend to agree that the less clutter in the viewport the better. However, since you can have multiple active viewports in the 3D view, I thought placing the button in each viewport makes more sense, rather than having a button in the menu where user would have to pick a viewport by an ID or something similar.

Anyway, the UI is still in kind of a prototype state. Maybe to solve the "issue" with multiple 3D viewports, there will be a single button in the toolbar and it will simply replicate the viewport that was used the last. Or each time the user interacts with a different viewport, the replication switches to it. This could be a bit trippy, though.

@OBKF

This comment has been minimized.

Copy link
Contributor

OBKF commented Apr 7, 2019

@SeleckyErik I was referring the other way lol.
I mean if you can play the game inside the editor, or the editor get's the state that your game you are at.
For example if you have to tweak a parameter you need to switch back and forth to the editor (unless you have dual monitor or linux desktop where you set game window ontop).

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Apr 7, 2019

@OBKF I think it was a deliberate design decision to not have a game running inside of the editor.

@Zireael07

This comment has been minimized.

Copy link
Contributor

Zireael07 commented Apr 8, 2019

@SeleckyErik I think at first, but then the stance changed. I know it has been attempted at least twice, but never got far enough to actually be tested.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Apr 8, 2019

Hmm... I didn't know that. Indeed it would be an interesting feature to have.

@OBKF

This comment has been minimized.

Copy link
Contributor

OBKF commented Apr 8, 2019

Most of the times the game runs differently from the editor, for example there is no get_viewport().get_camera() in the editor, so doing something with viewing camera won't be possible and can only be seen in game.
On the other hand, the editor have some slowdowns (mentioned in a few issues), therefore the performance (I suppose) won't be the same as a separated window, but its worth the time saving.
If this feature got implemented it should be optional and the user must have choice to run it in editor or separated window.

@reduz

This comment has been minimized.

Copy link
Member

reduz commented Apr 21, 2019

I think the PR is mostly good, but I think the way to override the camera is not so good. This is not your fault, but the fact there is not a clean way to do this. Let me see if I can add relevant functions to Viewport for this, to make your life easier

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Apr 21, 2019

If you can give me some pointers on what is not so well done and what would be your proposed solution, I can attempt implementing it myself. I can only imagine you have more than enough tasks on your to-do list.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented May 13, 2019

@reduz any update on this? Will you have time to work on this any time soon or explain me what needs to be changed? I will be working on the watches now, but afterwards I want to get back to this PR.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Jun 5, 2019

Addressed the mentioned issues: Reworked the 3D camera override API in the Viewport as discussed with @reduz. Moved the button to trigger the tool to the top toolbar as suggested by @groud. Also cleaned up the code so this should be now ready to merge.

@rxlecky rxlecky changed the title [WIP] Editor viewport camera replication Editor viewport camera replication Jun 5, 2019
@rxlecky rxlecky force-pushed the rxlecky:camera-replication branch 2 times, most recently from fb3c8ee to 74d8ab9 Jun 5, 2019
@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Jun 11, 2019

Squashed all commits

editor/plugins/canvas_item_editor_plugin.cpp Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Jun 11, 2019

Noted. Will make the changes and get back to you. :)

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Jun 20, 2019

I believe I addressed all the issues that you pointed out @reduz. The only thing I am not sure about is whether the SpatialIndexer in World needs to be aware of the override camera. If it does, it would be problematic since it keeps track of all cameras in form of Camera node pointers and I'm now only using Camera RID.

Also, I'd say that when the camera override is used, the overriding camera should be used for the spatial sound, rather than the in-game camera. However, since theSpatialSoundServer is currently disabled/not implemented, I did not implement this functionality. This will have to be additionally fixed once the spatial sound is implemented again.

@rxlecky rxlecky force-pushed the rxlecky:camera-replication branch from e95f8e0 to 1db536c Jun 25, 2019
@rxlecky rxlecky force-pushed the rxlecky:camera-replication branch 2 times, most recently from 15c684e to e4c2385 Aug 1, 2019
Copy link
Member

reduz left a comment

This is way better now, It's almost there :)

editor/plugins/spatial_editor_plugin.h Outdated Show resolved Hide resolved
scene/3d/camera.cpp Outdated Show resolved Hide resolved
String cmdstr = p_command[0];
if (!live_edit_funcs || !cmdstr.begins_with("live_"))

This comment has been minimized.

Copy link
@reduz

reduz Aug 5, 2019

Member

This is really nice clean up

This comment has been minimized.

Copy link
@rxlecky

rxlecky Aug 5, 2019

Author Contributor

I had to make the debugger a friend of SceneTree for this but I think this is the kind of scenarios where it should be used. However, a question I wanted to ask is whether all _live_edit functions couldn't be moved to the remote debugger? They all use the public API as far as I could see and have to do with debugging. And keeping the SceneTree as compcat as possible might be worth the effort.

@@ -1330,6 +1310,25 @@ void SceneTree::add_current_scene(Node *p_current) {
}
#ifdef DEBUG_ENABLED

static void _fill_array(Node *p_node, Array &array, int p_level) {

This comment has been minimized.

Copy link
@reduz

reduz Aug 5, 2019

Member

You can probably move all this back to the debugger so there's less clutter in SceneTree

scene/main/scene_tree.cpp Outdated Show resolved Hide resolved
scene/main/scene_tree.cpp Show resolved Hide resolved
editor/plugins/spatial_editor_plugin.cpp Show resolved Hide resolved
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Aug 28, 2019

Needs a rebase, and pending some changes based on @reduz's review.
@rxlecky Would you be able to do this in coming days? I'd like to merge this ASAP to have in 3.2-alpha1.

Once this PR is merged, #26219 should likely be rebased on top of it since the debugger is moving from core to scene.

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Aug 29, 2019

I'm currently in the process of moving to another country hence I was not very active in the past few weeks. I'll try to do this over the weekend but I can't promise anything.

There's still quite some work to be done on #26219 so it sure won't make it to alpha anyway.

@rxlecky rxlecky force-pushed the rxlecky:camera-replication branch from e4c2385 to 6f27dec Sep 3, 2019
@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Sep 3, 2019

Alright @akien-mga, pushed the changes now. Sorry for the delay. 🙁

The only point from @reduz's review that I did not address is offloading the live_edit functions from SceneTree to remote debugger. The reason is that Object is directly accessing some of the live_edit caches so I thought creating the direct Object -> ScriptDebuggerRemote coupling would be even worse. I can come back to this later and figure a way around this in another PR or, if @reduz thinks it would be fine, I can just move the coupling to remote debugger.

@rxlecky rxlecky force-pushed the rxlecky:camera-replication branch from 6f27dec to 0531b31 Sep 21, 2019
@rxlecky rxlecky force-pushed the rxlecky:camera-replication branch 2 times, most recently from a152824 to a2b5fc9 Oct 14, 2019
@rxlecky rxlecky requested a review from godotengine/documentation as a code owner Oct 15, 2019
@rxlecky rxlecky force-pushed the rxlecky:camera-replication branch 3 times, most recently from 6ff63be to 83f08c9 Oct 15, 2019
@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Oct 22, 2019

Summary of what was discussed at the Godot Sprint:

  • The PR should be good to merge now to get feedback on the tool from the users. Later when Pedro's PR (#29136) is ready for 4.0, small tweaks might be needed.

  • It would be cool if this worked even when the game is paused from the editor. I'm currently working on implementing this.

  • Currently the tool uses camera node icon. More distinct icon that illustrates the tool functionality better would improve readability. Maybe @Calinou could give me a hand here?

@rxlecky rxlecky force-pushed the rxlecky:camera-replication branch from 83f08c9 to e9a7694 Oct 23, 2019
@rxlecky rxlecky changed the title Editor viewport camera replication Game camera override Oct 23, 2019
Implemented uniform API in Viewport class to override 2D and/or
3D camera.

Added buttons in 2D and 3D editor viewport toolbars that override
the running game camera transform with the editor viewport camera
transform. Implemented via remote debugger protocol and camera
override API.

Removed LiveEditFuncs function pointers from ScriptDebugger class.
Since the debugger got access to the SceneTree instance (if one
exists), there is no need to store the function pointers. The live
edit functions in SceneTree are used directly instead. Also removed
the static version of live edit functions in SceneTree for the same
reason. This reduced the SceneTree -> Debugger coupling too since
the function pointers don't need to be set from SceneTree anymore.

Moved script_debugger_remote.h/cpp from 'core/' to 'scene/debugger/'.
This is because the remote debugger is now using SceneTree directly
and 'core/' classes should not depend on 'scene/' classes.
@rxlecky rxlecky force-pushed the rxlecky:camera-replication branch from e9a7694 to 8b0546d Oct 23, 2019
// This is for the camera override to stay live even when the game is paused from the editor
loop_time_sec = (OS::get_singleton()->get_ticks_usec() - loop_begin_usec) / 1000000.0f;
VisualServer::get_singleton()->sync();
if (VisualServer::get_singleton()->has_changed()) {
VisualServer::get_singleton()->draw(true, loop_time_sec * Engine::get_singleton()->get_time_scale());
}
Comment on lines +360 to +365

This comment has been minimized.

Copy link
@rxlecky

rxlecky Oct 23, 2019

Author Contributor

Made camera override work even when the game is paused from the editor.

@Calinou

This comment has been minimized.

Copy link
Member

Calinou commented Oct 28, 2019

@rxlecky What do you think about an icon like the one below? It's based on the CameraTexture icon, but with a larger camera icon and a thinner rectangle (1 pixel instead of 2 pixels).

1x: "Camera sync" icon (1x)

2x: "Camera sync" icon (2x)

Optimized SVG: icon_camera_sync.zip

@rxlecky

This comment has been minimized.

Copy link
Contributor Author

rxlecky commented Oct 28, 2019

@Calinou I'm not sure if that quite gets the idea across. I was thinking of something illustrating two cameras being linked together. But that seems like a difficult thing to fit in an icon. Maybe something simpler like a combination of camera icon and a panning tool would do?

obrázok

Not quite like this but you get the idea 😄

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Nov 8, 2019

It's now a bit late for 3.2 given the release freeze, but as this was reviewed extensively over the past few months by @reduz and we approved it at the Godot Sprint, let's make an exception.

@akien-mga akien-mga merged commit 621dc70 into godotengine:master Nov 8, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Nov 8, 2019

Thanks!

@aaronfranke

This comment has been minimized.

Copy link
Member

aaronfranke commented Nov 23, 2019

So far, only 2D camera replication works. 3D is WIP.

I assume this text is out of date, because this PR changes spatial_editor_plugin.cpp.

@Calinou

This comment has been minimized.

Copy link
Member

Calinou commented Nov 23, 2019

@aaronfranke Indeed, it works well in 3D from my testing. However, I noticed that camera FOV is only set if you set it while the preview is enabled. If you disable camera replication, change the camera FOV in the View settings and enable it again, it won't use the new FOV value (#33847).

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

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.