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

Propagate previously unused NOTIFICATION_WORLD_2D_CHANGED, make CanvasItem/CollisionObject2D use it #57179

Merged

Conversation

spacechase0
Copy link
Contributor

@spacechase0 spacechase0 commented Jan 25, 2022

(Master version of #52761)

I took into account the feedback on the previous PR (and put one comment there, since a suggested change didn't work).

New test project (converted one from #52423): LocationTest.zip (One small difference is I had to change the default environment background color for the 3D test so you could actually see things, since for some reason with master it doesn't render correctly, and everything is black instead. Not sure what the deal with that is.)

Bugsquad edit:

@spacechase0 spacechase0 requested review from a team as code owners January 25, 2022 03:06
@Chaosus Chaosus added this to the 4.0 milestone Jan 25, 2022
@akien-mga akien-mga changed the title Propogate previously unused NOTIFICATION_WORLD_2D_CHANGED, make CanvasItem/CollisionObject2D use it Propagate previously unused NOTIFICATION_WORLD_2D_CHANGED, make CanvasItem/CollisionObject2D use it Jan 25, 2022
@reduz
Copy link
Member

reduz commented Feb 6, 2022

So, this is actually really good and needed, the problem is that in 3D you have ENTER_WORLD and EXIT_WORLD notifications and this PR would make 2D work inconsistently with that.

That said, I think the functionality in this PR is better, and changing the 3D notifications to a single NOTIFICATION_WORLD_3D_CHANGED would be more fit, as in most cases you could just use enter/exit tree and handle the 3D world change separately.

So,would you be up to do this change on the 3D side too to make it remain consistent?

@spacechase0
Copy link
Contributor Author

Sorry for the huge delay in response on this - is changing 3D to match still something that is desired since Godot 4 is in beta now? Or should the 2D functionality here be made to match? Or neither?

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 16, 2023
scene/main/viewport.cpp Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
scene/main/viewport.h Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented May 1, 2023

IMO the PR is fine in the current form. It's too late to change the 3D side, so we'll have to live with inconsistency I guess.

@spacechase0
Copy link
Contributor Author

Should I squash the PR into 1 commit as well? Or at least the last 3 commits (since they just say "Update ", since I forgot to change them before committing in the web interface).

@KoBeWi
Copy link
Member

KoBeWi commented May 1, 2023

Squash everything into 1 commit.

@spacechase0 spacechase0 force-pushed the notify-world2d-changed-master branch from 12d4260 to 46e06ee Compare May 1, 2023 22:48
@spacechase0
Copy link
Contributor Author

Okay, should be good to go.

Comment on lines +3820 to +3821
if (v) {
if (v->world_2d.is_valid()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (v) {
if (v->world_2d.is_valid()) {
if (v && v->world_2d.is_valid()) {

Can be simplified.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this was written like this to be consistent with 3D:

Viewport *v = Object::cast_to<Viewport>(p_node);
if (v) {
        if (v->world_3d.is_valid() || v->own_world_3d.is_valid()) {
                return;
        }
}

Which would need to become if (v && (v->world_3d.is_valid() || v->own_world_3d.is_valid())) too.

@akien-mga akien-mga merged commit bbe05b6 into godotengine:master May 8, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga akien-mga changed the title Propagate previously unused NOTIFICATION_WORLD_2D_CHANGED, make CanvasItem/CollisionObject2D use it Propagate previously unused NOTIFICATION_WORLD_2D_CHANGED, make CanvasItem/CollisionObject2D use it May 10, 2023
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.

Separate Viewport/World2D with another viewport sharing the World2D causes physics errors, no rendering
7 participants