Propogate previously unused NOTIFICATION_WORLD_2D_CHANGED, make CanvasItem/Camera2D/CollisionObject2D use it #52761
Conversation
…sItem/Camera2D/CollisionObject2D use it to fix godot/godotengine#52423
There was a problem hiding this comment.
Thanks for contributing this fix!
It looks like it makes sense to handle world changes in 2D, although I haven't tested yet. I've left some comments on parts of the code that need to be a bit different from 3D logic, since entering/leaving worlds doesn't work the same in 2D and 3D.
Two other things that are needed before a possible approval:
-
Documentation needs to be updated
Here for the new notification: https://github.com/godotengine/godot/blob/master/doc/classes/CanvasItem.xml#L508
Here for the new function call: https://github.com/godotengine/godot/blob/master/doc/classes/CanvasItem.xml#L19 -
A master version of this PR is needed
What exactly was blocking you from making a master PR? Is about setting the branch, testing your changes, or a more specific issue with the code?
| @@ -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); | |||
| } | |||
There was a problem hiding this comment.
This should be only called if the previous world is valid, otherwise it's going to do some unnecessary extra processing.
|
|
||
| _setup_viewport(); | ||
| _update_scroll(); | ||
| } break; |
There was a problem hiding this comment.
Is this whole case really needed? It doesn't look like it's doing any world-related logic.
| if (get_script_instance()) { | ||
| get_script_instance()->call_multilevel(SceneStringNames::get_singleton()->_world_2d_changed, nullptr, 0); | ||
| } | ||
| } break; |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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.
| Physics2DServer::get_singleton()->area_set_transform(rid, global_transform); | ||
| } else { | ||
| Physics2DServer::get_singleton()->body_set_state(rid, Physics2DServer::BODY_STATE_TRANSFORM, global_transform); | ||
| } |
There was a problem hiding this comment.
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.
| Physics2DServer::get_singleton()->body_set_space(rid, space); | ||
| } | ||
|
|
||
| _update_pickable(); |
There was a problem hiding this comment.
Same with _update_pickable(), from what I can see it doesn't seem to depend on the world.
|
|
||
| _update_pickable(); | ||
|
|
||
| //get space |
There was a problem hiding this comment.
Extra comment to be removed.
akien-mga
force-pushed
the
3.x
branch
2 times, most recently
from
71cb8d3
to
c58391c
Compare
|
(Made a new PR that is for master, closing this one.) |
(Fixes #52423)
I'm not sure how do this in the master branch. I see in the comment in the PR template saying to do things for master first - if I could get some guidance on that, I would appreciate it.