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

Fix Camera2D is not working inside a MainScreenEditorPlugin #79867

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

WhalesState
Copy link
Contributor

@WhalesState WhalesState commented Jul 25, 2023

@WhalesState WhalesState requested a review from a team as a code owner July 25, 2023 02:28
@WhalesState
Copy link
Contributor Author

WhalesState commented Jul 25, 2023

just one more fix, draw screen/limits/drag margins are not updating if is editing in editor and inside a custom viewport, how to fix it ? by checking if (_is_editing_in_editor() || (custom_viewport && (custom_viewport->get_viewport() == get_tree()->get_edited_scene_root().get_viewport()))) ? i can fix it by adding an extra check if get_tree()->get_root() == get_tree()->get_edited_scene_root() but it won't work if inside a packed scene, this is tricky

@WhalesState
Copy link
Contributor Author

WhalesState commented Jul 25, 2023

I think removing _is_editing_in_editor() checks inside the 3 methods will fix it since it only calls queue_redraw() which will draw them in the next process frame, testing now and i hope it works as expected.
edit: my bad! i also need to check if Engine::get_singleton()->is_editor_hint() :) thought #ifdef TOOLS_ENABLED is enough to check for this.

edit: still not updating -_-

@Calinou Calinou added bug topic:editor topic:plugin topic:2d cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 25, 2023
@Calinou Calinou added this to the 4.x milestone Jul 25, 2023
@WhalesState
Copy link
Contributor Author

WhalesState commented Jul 25, 2023

After testing I have found that this has many benefits:

1- Camera2D works as expected in Editor edited scene root if not inside a SubViewport else it will directly update the SubViewport preview from the camera if it's enabled , and if disabled it will has no effect but only draw screen/limits/drag_margins will not be shown
2- inside a MainScreenEditorPlugin it will work as if inside game, which is amazing like Pixelorama for example can work directly inside a MainScreenEditorPlugin and get released as a godot addon!

if anyone can help me making draw screen/limits/drag margins be visible if inside a SubViewport and not enabled, note that you need to draw to the Subviewport and not the root viewport.

scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 25, 2023
scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

The approach overall makes sense, though I haven't reviewed or tested the changes yet.

One thing that you'll have to do eventually for this to be merged is to squash your commits into one. Please, make sure that the final commit has a short but descriptive message (the title of this PR is a good option). See this documentation, if you need help with squashing.

@WhalesState
Copy link
Contributor Author

i want to cry, i have done this git reset --soft HEAD~13 && git commit -m "Fix Camera2D is not working inside a MainScreenEditorPlugin" then forced a push

scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
@WhalesState
Copy link
Contributor Author

Thank you ^^ I've updated the code and tested inside Editor Plugin, this also fixes the other issue where white color was drawing black! I will push the changes now ^^

image

@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 8, 2023

Also I'm sorry about the refactor, just have got many warnings from CodeScene plugin about complex expressions. also the last edit makes the Camera works only inside a MainScreenEditorPlugin and no changes happens to it inside edited scene root, this is amazing and also it's not updating anymore! thank you ^^

@WhalesState
Copy link
Contributor Author

should i change is_part_of_edited_scene() to Node2D::is_part_of_edited_scene() ?

@AThousandShips
Copy link
Member

AThousandShips commented Aug 8, 2023

It is an editor build only method, you can only use it in editor only code, it's not a direct replacement

@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 8, 2023

can i make a function like this and use it instead ?

bool _is_editing_in_editor() const {
#ifdef TOOLS_ENABLED
    return is_part_of_edited_scene();
#endif
    return false;
}

@AThousandShips
Copy link
Member

AThousandShips commented Aug 8, 2023

Sure, though it should be;

bool Camera2D::_is_editing_in_editor() const {
#ifdef TOOLS_ENABLED
    return is_part_of_edited_scene();
#else
    return false;
#endif // TOOLS_ENABLED
}

Or you'll get errors by returning multiple times

Though I'm not sure about the exact best solution here, it feels pretty hacky

Edit: You can't use a local function like that, it has to be a member function to access a member method

Unless you do:

static bool _is_editing_in_editor(const Camera2D *p_camera) {
#ifdef TOOLS_ENABLED
    return p_camera->is_part_of_edited_scene();
#else
    return false;
#endif // TOOLS_ENABLED
}

And provide it with this but that doesn't seem clean

@WhalesState
Copy link
Contributor Author

one more const function isn't a big deal for this fix, right ?

bool Camera2D::_is_editing_in_editor() const {
#ifndef TOOLS_ENABLED
	return false;
#else
	return is_part_of_edited_scene();
#endif // !TOOLS_ENABLED
}

@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 8, 2023

using #ifndef instead just to unshadow the other return in vscode :D

@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 8, 2023

If you have "if - else", then there is no reason to use #ifndef, just use #ifdef and invert the cases. Having a wrapper method is fine here, yes.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 8, 2023

There is a bug, the camera borders no longer follow the node:

godot.windows.editor.dev.x86_64_0gCNP1oYgA.mp4

@WhalesState
Copy link
Contributor Author

WhalesState commented Aug 9, 2023

fixed, I could never have done this without your help. Thank you all and lets meet in another issue and we fix it together, I'm excited to learn more from you. ^^

Note: the editor may appear slow because it's built with optimize=none and is connected to vscode debugger.

09.08.2023_07.09.55_REC.mp4

scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
scene/2d/camera_2d.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit 8b3de35 into godotengine:master Aug 14, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@WhalesState
Copy link
Contributor Author

Thanks! And congrats for your first merged Godot contribution 🎉

Thank you, I'm so happy ^^. have a nice day 🎉.

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

Successfully merging this pull request may close these issues.

Camera2D in a viewport while running as an editor plugin doesn't work
6 participants