-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Expose editor viewports in EditorInterface #68696
Expose editor viewports in EditorInterface #68696
Conversation
95a7b55
to
b373a78
Compare
The C# mono build fails. What needs to change to allow this to call a static
|
This should fix the C#: diff --git a/modules/mono/editor/GodotTools/GodotTools/Ides/GodotIdeManager.cs b/modules/mono/editor/GodotTools/GodotTools/Ides/GodotIdeManager.cs
index 9df90ac608..8acd701647 100644
--- a/modules/mono/editor/GodotTools/GodotTools/Ides/GodotIdeManager.cs
+++ b/modules/mono/editor/GodotTools/GodotTools/Ides/GodotIdeManager.cs
@@ -76,7 +76,7 @@ namespace GodotTools.Ides
public async Task<EditorPick?> LaunchIdeAsync(int millisecondsTimeout = 10000)
{
- var editorId = (ExternalEditorId)(int)GodotSharpEditor.Instance.GetEditorInterface()
+ var editorId = (ExternalEditorId)(int)EditorPlugin.GetEditorInterface()
.GetEditorSettings().GetSetting("mono/editor/external_editor");
string editorIdentity = GetExternalEditorIdentity(editorId);
diff --git a/modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathManager.cs b/modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathManager.cs
index 60602a5847..fd7dcee221 100644
--- a/modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathManager.cs
+++ b/modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathManager.cs
@@ -13,7 +13,7 @@ namespace GodotTools.Ides.Rider
private static string GetRiderPathFromSettings()
{
- var editorSettings = GodotSharpEditor.Instance.GetEditorInterface().GetEditorSettings();
+ var editorSettings = EditorPlugin.GetEditorInterface().GetEditorSettings();
if (editorSettings.HasSetting(EditorPathSettingName))
return (string)editorSettings.GetSetting(EditorPathSettingName);
return null;
@@ -21,7 +21,7 @@ namespace GodotTools.Ides.Rider
public static void Initialize()
{
- var editorSettings = GodotSharpEditor.Instance.GetEditorInterface().GetEditorSettings();
+ var editorSettings = EditorPlugin.GetEditorInterface().GetEditorSettings();
var editor = (ExternalEditorId)(int)editorSettings.GetSetting("mono/editor/external_editor");
if (editor == ExternalEditorId.Rider)
{
@@ -81,7 +81,7 @@ namespace GodotTools.Ides.Rider
return riderPath;
}
- var editorSettings = GodotSharpEditor.Instance.GetEditorInterface().GetEditorSettings();
+ var editorSettings = EditorPlugin.GetEditorInterface().GetEditorSettings();
var paths = RiderPathLocator.GetAllRiderPaths();
if (!paths.Any())
|
b373a78
to
e2c4cf1
Compare
190b374
to
b19ab44
Compare
@raulsntos Excellent, thank you for the patch. I have applied it and built successfully. |
With #75694 merged, this needs to be updated to remove static changes. It also needs a rebase. Provided this is still desired, of course. |
b19ab44
to
4c1429a
Compare
It is desired. This has been rebased and static functions removed. |
} | ||
|
||
SubViewport *EditorInterface::get_editor_viewport_3d(int p_idx) const { | ||
ERR_FAIL_INDEX_V(p_idx, static_cast<int>(Node3DEditor::VIEWPORTS_COUNT), nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERR_FAIL_INDEX_V(p_idx, static_cast<int>(Node3DEditor::VIEWPORTS_COUNT), nullptr); | |
ERR_FAIL_INDEX_V(p_idx, int(Node3DEditor::VIEWPORTS_COUNT), nullptr); |
Assuming the casting is required, this should work too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this line directly from Node3DEditorViewport::get_editor_viewport()
. VIEWPORTS_COUNT is an unsigned int.
Isn't static_cast<int>
preferred for C++ rather than C cast style?
https://stackoverflow.com/questions/1609163/what-is-the-difference-between-static-cast-and-c-style-casting
If you confirm C style cast is desired, I'll make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen C cast used more often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nitpicky, but indeed we prefer C style casts throughout the codebase, unless there's a real ambiguity that a dedicated C++ casting expression would resolve.
We'd actually style it like this usually, which we find the most readable.
ERR_FAIL_INDEX_V(p_idx, static_cast<int>(Node3DEditor::VIEWPORTS_COUNT), nullptr); | |
ERR_FAIL_INDEX_V(p_idx, (int)Node3DEditor::VIEWPORTS_COUNT, nullptr); |
Edit: I see this was likely taken from:
Node3DEditorViewport *get_editor_viewport(int p_idx) {
ERR_FAIL_INDEX_V(p_idx, static_cast<int>(VIEWPORTS_COUNT), nullptr);
return viewports[p_idx];
}
so I guess there was a precedent for this cast :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I copied the line from the downstream function. I wasn't partial and was looking for firm direction. Now I know C style is preferred. Thanks for addressing the PR.
Are there any existing problems with this PR except a (trivial) code conflict? If not:
Apologize for inconvenience. |
@yslking Wait, why would you make a new PR? Edit: @yslking it's all good. It's also good to prompt the core devs to make a decision on work that is already done, reviewed, and approved. |
@TokisanGames. Ok. I see that this PR has been inactive for a long time, and there are too many PRs still open in the Godot project, so I want to do something to get this PR merged into master as soon as possible. Thanks for your explanation, I've closed the new PR. If you don't mind, as a newbie in the open source community, I would like to ask how long it usually takes the core team to review and merge such a PR? |
There is no "how long usually" in Godot repository. Some PRs get merged very fast, within weeks. Some languish for years. |
So some procedure and advice, but let's not get deeper into this as it is off topic to this:
|
fefffd8
to
110130b
Compare
Rebased and happy to do so again upon request from core devs. @YuriSizov @akien-mga Is there any resistance to merging this in 4.2? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Thanks! |
I may be missing something but can we not get the current viewport / camera ? Say I'm trying to create a planar reflection that I want to work in the editor, for it to work in all 4 views I need to retrieve the appropriate camera for each view. At the moment it doesn't appear that this is possible. |
It doesn't work to do |
Not only is it possible, the code at the top of this PR shows you exactly how to get all 4 editor viewports and their cameras. Use Godot 4.2. Make a new scene with a node. Put that script on it, and you will see the output shown. |
@AThousandShips @TokisanGames Yes, you can get all viewports and all cameras. But I only want to retrieve the current camera relative to the view, so view 1 gets the camera 1, view 2 gets the camera 2 etc.
|
That's not implemented, please open a proposal for it as there's no current system for it as far as I can see easily accessible |
Ok thanks, just wanted to make sure I wasn't missing something before opening a proposal. |
You know the value of i. viewport 0 : i = 0, viewport 3 : i = 3. You know exactly which camera is in which viewport. Also each camera is a child of each viewport, so it's not possible to have any ambiguity. They are attached to each other. You also know the viewport and camera nodes, which means you can connect to signals and functions of those and surrounding nodes. They all report signals. For instance you could have printed the parental tree of an editor camera: ...and discovered that the viewport is in a SubViewportContainer which is a Control node, which sends out signals every time the mouse enters it, anytime there is keyboard input, or it's resized, or a bunch of other things. I don't think you need another proposal. You just need to learn the API better, which is done through study of the docs and experimentation over time. |
@TokisanGames I do not follow. It's not a problem of knowing which camera is attached to which viewport. It's about finding the camera that is responsible for rendering each of the 4 views when using the "split" view in the editor. Yes I—the user—know which value
How is the script here supposed to know though which camera the scene is being viewed (which will then allow me to get the current viewport texture to use on the plane) ? This is not a problem with the PR per say and am not criticising it in any way. I am learning Godot and thought getting the camera of the current view would be rather trivial. It's just turning out to be a lot more complex than I initially thought. I have spent the last two days looking through the API and examples but have hit this roadblock, and seeing as this PR is very close to what I am looking for that I may be able to find an answer here. |
A node can run get_viewport(). However, I think you have bigger problems in your conception of this scenario than figuring out the viewport. You don't have 4 scenes or Worlds. You have 1. Your one object will run that script once. It won't run it 4 times, one for each camera/vp. If the mesh printed the viewport number on its texture, it would show 0 (or 1) on all 4, and that would be working properly. After it runs once, it then stops. Then if you intend to put the viewport texture on the object, you'll have another problem. You have 1 mesh instance, not 4. You have one material with one texture. If it ran properly the first time and grabbed the texture from the first viewport, it would show the first viewport texture in all of the other windows. https://docs.godotengine.org/en/stable/tutorials/rendering/viewports.html#capture The only way I can think of to show the texture differently in each viewport is to use a screen space shader and perhaps that could copy it for each vp, but I'm not positive that would work. Otherwise, if you want to have separate materials and textures per viewport, you need 4 separate Worlds and mesh instances and materials. |
This PR grants gdscript access to the editor viewports, which grant access to the cameras, as discussed in godotengine/godot-proposals#1302.
For 3D Viewports
EditorInterface.get_editor_viewport_3d(p_index) -> SubViewport
provides access to the 3d editor viewports.The cameras can already be accessed with
Viewport.get_camera_3d()
(no change).For 2D Viewports
EditorInterface.get_editor_viewport_2d() -> SubViewport
provides access to the 2D viewport.There is no 2D editor camera per @KoBeWi, as view transforms are done directly. The viewport transform can be accessed as shown in the example below.
Example Usage
This script shows accessing the 2D viewport and all 4 3D cameras. The positions are printed to the console as the 2D or any of the 4 3D viewport cameras are moved around.
Output
Update: This PR has been updated post #75694