godot icon indicating copy to clipboard operation
godot copied to clipboard

Propagate previously unused NOTIFICATION_WORLD_2D_CHANGED, make CanvasItem/CollisionObject2D use it

Open spacechase0 opened this issue 2 years ago • 2 comments

(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.)

spacechase0 avatar Jan 25 '22 03:01 spacechase0

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?

reduz avatar Feb 06 '22 07:02 reduz

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?

spacechase0 avatar Oct 25 '22 17:10 spacechase0

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.

KoBeWi avatar May 01 '23 20:05 KoBeWi

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).

spacechase0 avatar May 01 '23 22:05 spacechase0

Squash everything into 1 commit.

KoBeWi avatar May 01 '23 22:05 KoBeWi

Okay, should be good to go.

spacechase0 avatar May 01 '23 22:05 spacechase0

Thanks! And congrats for your first merged Godot contribution :tada:

akien-mga avatar May 08 '23 14:05 akien-mga