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

GridMap enhancement - A signal or callback for on_cell_changed() #11855

Closed
Tracked by #10418
Scott3730 opened this issue Oct 5, 2017 · 7 comments
Closed
Tracked by #10418

GridMap enhancement - A signal or callback for on_cell_changed() #11855

Scott3730 opened this issue Oct 5, 2017 · 7 comments

Comments

@Scott3730
Copy link

The GridMap node would be an order of magnitude more powerful if I could have an editor script be able to detect whenever a cell has changed.

This could be achieved by any of a number of options. I'm not sure which would be the most appropriate method:

  • A notification that i could check for in _notification()
  • A signal (not sure if having signals that fire in editor/tool mode would be appropriate?)
  • A method that could be overridden in a tool mode script that extends GridMap, maybe _on_cell_changed(cell_pos)

With this simple addition, I could write an auto-tile editor script in GDScript that I could share with everyone. You could have variation in tiles occur automatically; ie, place a tree tile and have a random one of six tree models spawn. You could have tiles automatically rotate in specific ways.

There would be lots of possibilities opened up by this.

@groud groud added this to the 3.0 milestone Oct 5, 2017
@Zylann
Copy link
Contributor

Zylann commented Oct 5, 2017

Keep in mind that if you do auto-tiling, it implies changing neighbor tiles to connect, and that will trigger on_cell_changed too, because that signal fires anyways without knowing what happens.
For tile variations, it also means that if you set the tile, it will trigger the signal again and you'll get an infinite loop unless you work around with some boolean, assuming your tool is the only one thing listening to that signal... IMO, in the context of auto-tiling and variations, such a signal doesn't sound like a clean idea.

@Scott3730
Copy link
Author

what about _on_cell_change(Vector3 cell_pos, int new_id) ? or old_id, or both. I'm not sure which would be more useful. But there has to be a solution here somewhere. Or possibly if the engine detected when the new id was different from the old one, and only fired if true, that would stop any infinite loops, and if you're autotiling, only the cells one space away from the edge being tiled would receive the signal, because further back, the values wouldn't change.

@Zylann
Copy link
Contributor

Zylann commented Oct 5, 2017

Still in the use case of auto-tiling, I was more thinking about a different path to set tiles in an auto-tiling way, something more specialized than a generic "a tile changed". I remember there was an existing issue where people were discussing this?
As soon as you think about auto-tiling, there is a distinction to be made between "master tiles" and "final tiles". For example, you may place grass, or road (master tile), but it can place one in many grass tiles, or modify neigbors to connect roads in various ways (final tiles). A "final" tile can be considered a "master" tile though, as long as there is only one of a given kind. The thing is, the master tile you place might not be the one you get because of auto-tiling.

Note: I read this is for GridMap, but what I mean is also valid for TileMap ;)

@kubecz3k kubecz3k changed the title Feature Request: GridMap enhancement - A signal or callback for on_cell_changed() GridMap enhancement - A signal or callback for on_cell_changed() Oct 5, 2017
@Scott3730
Copy link
Author

Scott3730 commented Oct 6, 2017

Yes! Actually, when making an autotile system, i reserve the first handful of tiles in a set for "air, material 1, material 2, etc", rather than the 32 different tiles / orientations that could represent them. So you paint in materials, and let the auto tile script define the actual tiles. I believe this is exactly what you're also describing. 😄

It would obviously be ideal to have this entire concept baked right into the engine implementation of GridMap (but that is a lot of work, that's why i'm not asking for it). But with a simple notification of some sort that i can respond to in script, i can hack together something that works.

As long as it is either only called when a cell's index actually changes, or is called with the old/new state passed as a parameter.

@Zylann
Copy link
Contributor

Zylann commented Oct 6, 2017

Yeah that's my point: on_cell_changed doesn't know if the change is the one you want to place or the one done by auto-tiling, because it's too generic. It may help hack something indeed, but you'll have to deal with the limitation unless Godot gets aware of auto-tiling ;)

@akien-mga
Copy link
Member

akien-mga commented Sep 4, 2020

There have been various PRs for this (current one that could be merged is #33832), but I'm still unclear about the use case for this signal.

See discussion in #40044 (comment).

Signals are normally not emitted for actions that are taken knowingly by the developer, as you can emit your own signal at the same time as you change the cell yourself. Similarly, TileMap doesn't emit any signal when you change cells, as this is something that is always done by yourself.

Edit: Re-reading the OP and reading #33832 (comment) I see that this could be useful for tool scripts to act based on changed done from the GridMap editor plugin, and not in-game logic.
I guess that might make sense, unless we prefer to go the EditorPlugin route and expose a signal through the GridMap editor plugin itself. Whatever is decided, this is likely relevant for TileMap too. CC @groud

@akien-mga
Copy link
Member

We rediscussed this today in a PR review meeting, and the conclusion is pretty much the same as what I commented above. I do see a use case for this but that's something which requires more design discussion to be implemented in a similar way for both TileMaps and GridMaps. As I understand it the only use case is for tool scripts/editor plugins to implement things like auto-tiling, in which case that's something which would likely be best implemented via EditorPlugin or similar APIs, instead of in the core GridMap.

I think the discussion should be moved to https://github.com/godotengine/godot-proposals to assess such a proposal, see what's the "right" design for this. Then the implementation should be fairly straightforward once we reach consensus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants