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.

Sign up for GitHub

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

Jump to bottom

Propogate previously unused NOTIFICATION_WORLD_2D_CHANGED, make CanvasItem/Camera2D/CollisionObject2D use it #52761

Closed
wants to merge 1 commit into from
Closed

Propogate previously unused NOTIFICATION_WORLD_2D_CHANGED, make CanvasItem/Camera2D/CollisionObject2D use it #52761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions scene/2d/camera_2d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,18 @@ void Camera2D::_notification(int p_what) {
viewport = nullptr;

} break;
case NOTIFICATION_WORLD_2D_CHANGED: {
if (is_current()) {
if (viewport && !(custom_viewport && !ObjectDB::get_instance(custom_viewport_id))) {
viewport->set_canvas_transform(Transform2D());
}
}
remove_from_group(group_name);
remove_from_group(canvas_group_name);

_setup_viewport();
_update_scroll();
} break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this whole case really needed? It doesn't look like it's doing any world-related logic.

#ifdef TOOLS_ENABLED
case NOTIFICATION_DRAW: {
if (!is_inside_tree() || !Engine::get_singleton()->is_editor_hint()) {
Expand Down
8 changes: 8 additions & 0 deletions scene/2d/canvas_item.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,13 @@ void CanvasItem::_notification(int p_what) {
case NOTIFICATION_VISIBILITY_CHANGED: {
emit_signal(SceneStringNames::get_singleton()->visibility_changed);
} break;
case NOTIFICATION_WORLD_2D_CHANGED: {
_exit_canvas();
_enter_canvas();
if (get_script_instance()) {
get_script_instance()->call_multilevel(SceneStringNames::get_singleton()->_world_2d_changed, nullptr, 0);
}
} break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like this whole block is necessary (but I could be missing something).

From what I understand, only this modified code from _enter_canvas() seems really necessary (to be tested):

if (!canvas_layer) {
	RID canvas = get_viewport()->find_world_2d()->get_canvas();
	VisualServer::get_singleton()->canvas_item_set_parent(canvas_item, canvas);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just those lines you suggested did not work (on master, working on this now - opening a new PR soon). I'm not sure what in _exit_canvas()/_enter_cavnas() makes it work, but trying just the snippet in your comment made it where the circle under the character did not move with it graphically.

}
}

Expand Down Expand Up @@ -1199,6 +1206,7 @@ void CanvasItem::_bind_methods() {
BIND_CONSTANT(NOTIFICATION_VISIBILITY_CHANGED);
BIND_CONSTANT(NOTIFICATION_ENTER_CANVAS);
BIND_CONSTANT(NOTIFICATION_EXIT_CANVAS);
BIND_CONSTANT(NOTIFICATION_WORLD_2D_CHANGED);
}

Transform2D CanvasItem::get_canvas_transform() const {
Expand Down
21 changes: 21 additions & 0 deletions scene/2d/collision_object_2d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,27 @@ void CollisionObject2D::_notification(int p_what) {
Physics2DServer::get_singleton()->body_attach_canvas_instance_id(rid, 0);
}
} break;

case NOTIFICATION_WORLD_2D_CHANGED: {
Transform2D global_transform = get_global_transform();

if (area) {
Physics2DServer::get_singleton()->area_set_transform(rid, global_transform);
} else {
Physics2DServer::get_singleton()->body_set_state(rid, Physics2DServer::BODY_STATE_TRANSFORM, global_transform);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think setting the transform is really necessary (unless I'm missing something).

Only setting the new space from the 2D world should be necessary.


RID space = get_world_2d()->get_space();
if (area) {
Physics2DServer::get_singleton()->area_set_space(rid, space);
} else {
Physics2DServer::get_singleton()->body_set_space(rid, space);
}

_update_pickable();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with _update_pickable(), from what I can see it doesn't seem to depend on the world.


//get space
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra comment to be removed.

} break;
}
}

Expand Down
27 changes: 27 additions & 0 deletions scene/main/viewport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,6 +1017,10 @@ void Viewport::set_world_2d(const Ref<World2D> &p_world_2d) {

_update_listener_2d();

if (is_inside_tree()) {
_propagate_world_2d_changed(this);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be only called if the previous world is valid, otherwise it's going to do some unnecessary extra processing.


if (is_inside_tree()) {
current_canvas = find_world_2d()->get_canvas();
VisualServer::get_singleton()->viewport_attach_canvas(viewport, current_canvas);
Expand Down Expand Up @@ -1057,6 +1061,29 @@ void Viewport::_propagate_enter_world(Node *p_node) {
}
}

void Viewport::_propagate_world_2d_changed(Node *p_node) {
if (p_node != this) {
if (!p_node->is_inside_tree()) { //may not have entered scene yet
return;
}

if (Object::cast_to<CanvasItem>(p_node)) {
p_node->notification(CanvasItem::NOTIFICATION_WORLD_2D_CHANGED);
} else {
Viewport *v = Object::cast_to<Viewport>(p_node);
if (v) {
if (v->world_2d.is_valid()) {
return;
}
}
}
}

for (int i = 0; i < p_node->get_child_count(); i++) {
_propagate_world_2d_changed(p_node->get_child(i));
}
}

void Viewport::_propagate_viewport_notification(Node *p_node, int p_what) {
p_node->notification(p_what);
for (int i = 0; i < p_node->get_child_count(); i++) {
Expand Down
1 change: 1 addition & 0 deletions scene/main/viewport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ class Viewport : public Node {

void _propagate_enter_world(Node *p_node);
void _propagate_exit_world(Node *p_node);
void _propagate_world_2d_changed(Node *p_node);
void _propagate_viewport_notification(Node *p_node, int p_what);

void _update_stretch_transform();
Expand Down
1 change: 1 addition & 0 deletions scene/scene_string_names.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ SceneStringNames::SceneStringNames() {
_exit_tree = StaticCString::create("_exit_tree");
_enter_world = StaticCString::create("_enter_world");
_exit_world = StaticCString::create("_exit_world");
_world_2d_changed = StaticCString::create("_world_2d_changed");
_ready = StaticCString::create("_ready");

_update_scroll = StaticCString::create("_update_scroll");
Expand Down
1 change: 1 addition & 0 deletions scene/scene_string_names.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class SceneStringNames {
StringName _process;
StringName _enter_world;
StringName _exit_world;
StringName _world_2d_changed;
StringName _enter_tree;
StringName _exit_tree;
StringName _draw;
Expand Down