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

Add a way to know when a TileMapLayer's cell is modified #96188

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

Khusheete
Copy link
Contributor

@Khusheete Khusheete commented Aug 27, 2024

Hi !

This PR adds a callback inside the TileMapLayer that gets called when cells are updated. This was discussed in #96078, which was a previous attempt at implementing this feature.

@akien-mga
Copy link
Member

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

@Mickeon Mickeon requested review from groud and Mickeon August 27, 2024 22:12
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Documentation is okay, it's consistent with what groud wrote in the past.

@jeffbdavenport
Copy link

jeffbdavenport commented Oct 6, 2024

This has been pending for over a month can we have someone look at this that can move this forward? @Khusheete @groud

I made a similar PR as well because I had difficulty finding this PR #97896 I would be happy with any implementation

@Mickeon
Copy link
Contributor

Mickeon commented Oct 26, 2024

The checks on macOS have failed and I am not sure why. This PR may need a rebase, preferably squashing the commits, and then it's very good to merge.

@AThousandShips
Copy link
Member

It needs a rebase indeed

@Khusheete Khusheete requested review from a team as code owners October 26, 2024 12:57
@Calinou Calinou removed request for a team October 26, 2024 14:52
@Khusheete
Copy link
Contributor Author

Oops, I think I messed up the rebase.

Ok, I fixed it !

Comment on lines 30 to 37
Called when this [TileMapLayer]'s cells need an internal update. This update may be caused from individual cells being modified or by a change in the [member tile_set] (causing all cells to be queued for an update). The first call to this function is always for initializing all the [TileMapLayer]'s cells. [param coords] contains the coordinates of all modified cells, roughly in the order they were modified. [param forced_cleanup] is [code]true[/code] when a cleanup is required. This usually means that the [TileMapLayer] has either left the canvas, has left the tree or has been freed. It may also be [code]true[/code] if an update is requested when either [member tile_set] is [code]null[/code], [member enabled] is [code]false[/code] or this [TileMapLayer] is outside of the [SceneTree]. See also [method update_internals].
[b]Warning:[/b] Implementing this method may degrade the [TileMapLayer]'s performance.
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean when "a cleanup is required"? You do explain well when and why it happens, but you don't explain how the user should handle this.

Copy link
Member

@groud groud Oct 29, 2024

Choose a reason for hiding this comment

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

A "cleanup" is needed when the TileMapLayer internals should be cleaned up. That mostly means:

  • When layer is disabled
  • The TileSet is null
  • The layer is not visible
  • The TileMapLayer quits the tree
  • The node is freed

@Khusheete Khusheete force-pushed the tilemaplayer-cell-update branch from 0fc5b69 to b168ffa Compare October 29, 2024 11:34
@akien-mga akien-mga changed the title Add a way to know when a TileMapLayer's cell is modified [v2] Add a way to know when a TileMapLayer's cell is modified Nov 11, 2024
@akien-mga akien-mga force-pushed the tilemaplayer-cell-update branch from b168ffa to e541c90 Compare November 11, 2024 12:28
@akien-mga
Copy link
Member

akien-mga commented Nov 11, 2024

Force pushed an amend to fix the commit message which was mangled, and properly fix author credits.

…eMapLayer's cells are updated

Made `_update_cells` a hook into the `TileMapLayer`'s internal update
@akien-mga akien-mga force-pushed the tilemaplayer-cell-update branch from e541c90 to d92f5e5 Compare November 11, 2024 12:29
@Repiteo Repiteo merged commit 8034575 into godotengine:master Nov 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 11, 2024

Thanks! Congratulations on your first contribution! 🎉

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.

9 participants