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

TileMap: Update erase_cell to also delete instantiated scenes #72456

Closed
wants to merge 11 commits into from

Conversation

Sch1nken
Copy link
Contributor

@Sch1nken Sch1nken commented Jan 31, 2023

As mentioned here: #69596

The current implementation has two potential flaws. I've thought about possible fixes but would like to get some feedback first before implementing them:

  1. If a Node (instantiated scene) gets moved to another place in the tree (i.e. not a direct child of the TileMap anymore) OR the node gets renamed OR the node gets freed by the user, the current implementation can NOT remove the scene anymore (it should however be able to free-up the internal cell reference).
  2. It would be possible to connect to the exited_tree() signal of the nodes to mitigate some of these issues. This could be an optional parameter for set_cell() ("listen to lifetime events" = yes/no, or something similiar). Once the node gets removed/renamed/etc the tilemap could update its reference (or remove it depending on what happened to the node).

So right now the user should only add/remove cells through set_cell()/erase_cell() and never manually remove/move/rename any nodes that the tilemap created.

So the question is: Is it the TileMaps job to update references to scenes when they are modified through other means than using the TileMaps interface?

Bugsquad edit: Closes #69596

@Sch1nken Sch1nken requested a review from a team as a code owner January 31, 2023 15:14
@groud
Copy link
Member

groud commented Jan 31, 2023

So the question is: Is it the TileMaps job to update references to scenes when they are modified through other means than using the TileMaps interface?

No. There are hundreds of scenarios where a TileMap's internal scenes could be modified one way or another. We don't disallow completely editing scenes instantiated by a TileMap, as I guess it might be useful to workaround some limitations, but it's the user's responsibility not to mess up here.

We cannot cover all situations where users would do weird things, like renaming an instantiated scene, moving it around, etc... It's simply easier to assume that, if a user do something like that, the TileMap node might not be able to recover from that state.

This should however be properly documented though.

@Sch1nken
Copy link
Contributor Author

Good. In that case the pull request could suffice (if up to standards).

@groud
Copy link
Member

groud commented Jan 31, 2023

Sorry but I don't think the PR is right. The calls to _erase_quadrant or _make_quadrant_dirty should automatically free those scenes. The bug is likely there.

@Sch1nken
Copy link
Contributor Author

I think I see what you mean, yes. From what I gathered the only way scenes are deleted is when instantiated_scenes.is_empty() == true on line 1873.

But there is only one call to clear the instantiated_scenes set (inside _tile_set_changed). This would also remove all scenes and not just single ones removed by erase_cell()/set_cell().

So there currently is no good way to identify scenes that are to be deleted. I'll see what I can come up with.

@ChaoticLeah
Copy link

until this is merged I have been reading the tilemap godot source code and found a hack to make it roughly work. Probably way worse, and it is just a hack, but if you really need it to work now here you go:

func _clear_tilemap_cache(tilemap):
    for n in tilemap.get_children():
        tilemap.remove_child(n)
        n.queue_free()
    tilemap.tile_set.add_source(tilemap.tile_set.get_source(0), -1)

I will add if you are doing raycasting or something and its not hitting like expected then you would want to store the tilemaps children and delete them a frame later when its re-instanced (remember to make sure it hasn't already been removed from the scene)
Here is a implementation of that, but this possibly has some other issues:

var tiles_to_delete = {}
var saved_tilemap
var delete_at_frame = -1
#YOU HAVE TO CALL THIS AFTER EVERY SET CELL
# SHOULD BE OK AFTER https://github.com/godotengine/godot/pull/72456 is merged
func clear_tilemap_cache(tilemap):
    tiles_to_delete = tilemap.get_children()
    saved_tilemap = tilemap
    delete_at_frame = Engine.get_frames_drawn() + 1
    tilemap.tile_set.add_source(tilemap.tile_set.get_source(0), -1)

#Deleting it later once the next batch of tiles is already instanced makes it so that
#The raycasts dont fail
func _process(delta):
    if delete_at_frame < Engine.get_frames_drawn() and delete_at_frame != -1:
        for n in tiles_to_delete:
#            Make sure it has not already been deleted, if so skip it
            if !weakref(n).get_ref():
                continue;
            saved_tilemap.remove_child(n)
            n.queue_free()
        delete_at_frame = -1
        tiles_to_delete = {}

For both of these you will need to call _clear_tilemap_cache every time you modify the tilemap. Hopefully this helps someone, and keep in mind I think all this code should be able to be removed after this is merged but you should make sure of that before totally ripping it out, and also this is kinda experimental and hacky, but it works for me. Probably shouldn't use it if you want 100% sure it wont crash, but do your own testing, its pretty stable for me.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jun 23, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Jul 26, 2023

Needs rebase. I tested the PR and it doesn't seem to work, i.e. the scene is not removed. The recent changes to TileMap layers might've broken it.

EDIT:
I think it will work again once #79941 is merged.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

See above

@YuriSizov YuriSizov changed the title Tilemap update erase_cell() to also delete instantiated scenes TileMap: Update erase_cell to also delete instantiated scenes Jul 27, 2023
scene/2d/tile_map.cpp Outdated Show resolved Hide resolved
@joshmitcho
Copy link

Is this fix coming soon? It seems a lot of people are having the same issue

@KoBeWi
Copy link
Member

KoBeWi commented Aug 9, 2023

@Sch1nken Can you rebase the PR and apply changes?

@Sch1nken
Copy link
Contributor Author

Sch1nken commented Aug 9, 2023

Hey, can do. Don't have time today but can look into it tomorrow hopefully.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Works correctly now, but you still need to rebase and squash the PR.
Here's the rebased code:

		// Find node in scenes and remove it.
		HashMap<Vector2i, String>::Iterator entry = q.scenes.find(pk);
		if (entry != q.scenes.end()) {
			String scene_name = entry->value;
			Node *scene = tile_map_node->get_node_or_null(scene_name);
			if (scene) {
				scene->queue_free();
				instantiated_scenes.erase(Vector2i(pk.x, pk.y));
			}
		}

@Sch1nken
Copy link
Contributor Author

Currently working on it. Not much experience with rebasing and apparently rebased before pulling master.

Currently trying to resolve the conflict... Sorry for the delay.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 11, 2023

The code has moved around a lot. It's better if you accept the current code (i.e. on the master branch) when resolving conflict and then apply your changes again. You can use my snippet as a reference how to fixes compile errors that will emerge, because the original code will not work after the recent TileMap changes.

Riteo and others added 3 commits August 11, 2023 21:32
Integer scaling is achieved (after aspect expansion) by "lying" to the
stretching code about the window's size, telling it that it's always an
integer multiple of the viewport so that it only gets stretched to an
integer factor.

This approach works with all stretch and aspect modes and doesn't
require handling for each, only requiring to "loosen up" some
self-excluding conditions (in other words, replacing some `else if`s
with just `if`s) regarding viewport offset and margin calculation (black
bars).

Includes a tiny usability change that adds a range hint for the content
scale factor between 0.5 to 8.0.

Co-Authored-By: Hugo Locurcio <hugo.locurcio@hugo.pro>
@Sch1nken Sch1nken requested review from a team as code owners August 11, 2023 19:39
@Sch1nken
Copy link
Contributor Author

I don't think I've made the correct steps (rebasing, squashing etc). But my local git says everything is up to date while GitHub mentions 8 commits ahead and 10 commits behind from master...

@Sch1nken
Copy link
Contributor Author

Okay, I feel like I borked my PR/Git history beyond my comprehension. To not delay this any further: Should I just open a fresh new PR with the necessary changes? Or: If someone is willing to do it themselves, go for it.

I'm really struggling fixing my local git history. Rebasing and squashing are pretty new to me and it would take longer for me to understand it right now (and fix my mess) than to just start over.

@akien-mga
Copy link
Member

Superseded by #80658.

@akien-mga akien-mga closed this Aug 15, 2023
@akien-mga akien-mga modified the milestones: 4.2, 4.x Aug 15, 2023
@AThousandShips AThousandShips removed this from the 4.x milestone Aug 15, 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.

erase_cell() does not erase scene tiles cells