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

[3.x] Fix CanvasItem not exiting its canvas group on canvas exit #63234

Merged

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Jul 20, 2022

All CanvasItems not parented to another CanvasItem within the given canvas are added to the group specific for this canvas (on entering the canvas). However, they were not removed from such group on canvas exit which could result in unexpected/strange state (and thus behavior) like: CanvasItem being a child of another CanvasItem could still belong to a canvas group, or CanvasItem could belong to many different canvas groups etc.
Now they should be correctly removed from the canvas group on exiting the canvas.

Fixes #63183.

In action (MRP from #63183):
Scene / after ready():
Godot_v3 5-rc6_win64_OD0wa8cuFN / Godot_v3 5-rc6_win64_qaSCgFu1Sk

  • 3.5.rc6:
    Godot_v3 5-rc6_win64_qtbs0lgep9
    Fixed behavior by manual removal from the group:
    52FAopsbdY

  • This PR:
    v7dgQV7z7Q

@kleonc kleonc added this to the 3.5 milestone Jul 20, 2022
@kleonc kleonc requested a review from a team as a code owner July 20, 2022 10:47
@kleonc kleonc changed the title [3.x] Fix CanvasItem not existing its canvas group on canvas exit [3.x] Fix CanvasItem not exiting its canvas group on canvas exit Jul 20, 2022
@kleonc kleonc force-pushed the canvas-item-remove-from-canvas-group-3x branch from e985b47 to 558b96f Compare July 20, 2022 10:50
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

I've not done anything with canvas groups, but this looks fine to me.

I was having a look at compressing some of the bloat in canvas item and now looking at it, I'm not sure we need to be storing a string per canvas item for this, but that can be for another PR.

@akien-mga akien-mga merged commit f0fabec into godotengine:3.x Jul 20, 2022
@akien-mga
Copy link
Member

Thanks!

@kleonc kleonc deleted the canvas-item-remove-from-canvas-group-3x branch July 20, 2022 19:35
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.

3 participants