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 TileProperties, generalize EditorPropertyResource #23864

Closed
wants to merge 1 commit into from

Conversation

willnationsdev
Copy link
Contributor

@willnationsdev willnationsdev commented Nov 20, 2018

This PR does several things.

  1. The TileSet's scripted properties are now displayed in the Inspector. This enables TileMaps to see this information in the new EditorInspector when they expand out the sub-inspector.
  2. The TileSet now has a meta_properties Dictionary that is exported to the Inspector. It likewise can be edited from a TileMap just by expanding the sub-inspector.
  3. When using the new TileSet Editor, the TileSetContextEditor's inspector window will display the scripted properties and the meta_properties Dictionary just like in the TileMap. It also displays the script for the TileSet so that it can be assigned a value without having to open a separate Inspector.
  4. When a tile is selected, a new property is present at the bottom: TileProperties. This is a new resource type that can be inherited by scripts. If users create a TileProperties object and give it a script (the tile properties script is ALSO exposed to the TileSetContextEditor), then they can easily define custom properties per-tile and share them between tiles by re-using the same resource object.
  5. A tiles_default_script property has been added to the TileSet class and exposed to the Inspector. If one sets a script to this value, then all created tiles will automatically create an instance of a TileProperties sub-resource and assign that script to it, guaranteeing that all tiles have access to those properties.
  6. I also noticed that the EditorPropertyResource class wasn't taking into account script classes when determining what creation options to display, so I added a section that adds them to the potential inheritors.
  7. The EditorPropertyResource class was still using the old icon lookup algorithm, so I swapped out all of the logic for just using the new get_class_icon method.

This should take care of a great many different Issues:

  1. Closes Expose exported script variables in tileset editor. #23305. This exposes a TileSet's scripted properties, and then some.
  2. Related to Giving tiles a property dictionary #12634. While it doesn't directly add a Dictionary to each tile, it does provide a Resource which itself can contain a Dictionary. It also adds a Dictionary to the Tileset (which, if keys were combined with strings, could serve as an alternative, i.e. meta_properties[str(id) + "/property"] = "value". I THINK we could close it with this PR.
  3. Related to Add meta dictionary to individual tiles #23714. ^ This Issue is basically identical. Same reasoning.
  4. Related to Suggestion: Tile node for Tile Maps #15583. With this PR, combined with implementing Scene-tiles for tilemaps #9180, I think we'd have all of the needs associated with this PR met. As such, I don't really know whether it would still be necessary to implement a "Tile node".

Here are some screenshots from my build, using the below scripts:

# tileset.gd
extends TileSet
export var units_per_tile = 1

# tile.gd
extends TileProperties
export var type = "forest"
export var movement = 2
export var defense = 3

TileMap Inspector:
tilemap

TileSetContextEditor:
tilesetcontexteditor

Edit: I have just modified it so that the meta_properties value is displayed as "Meta Properties" in the TileSetContextEditor instead of as "Meta" (that was a mistake).

@xDGameStudios
Copy link
Contributor

xDGameStudios commented Nov 20, 2018

Thank you so much!! :) this is a great addition to the tiles functionality!! the default script idea is neat and the dictionary to the tileset instead of per-tile makes way more sense!! :)

Why is there still a non successful check in Travis?! the merge is near never the less, i wish I new more to contribute to this (godot) GuiHub project

@willnationsdev
Copy link
Contributor Author

@xDGameStudios The unsuccessful check in Travis was because my Visual Studio, for whatever reason, replaced a whole bunch of \t\t with (3 spaces), and when I was fixing things there was a single return statement that had one extra space after its tab. As a result, it didn't conform to the clang-format rules of the godot repository and failed the static check. I just fixed it. The next CI build should be all clear. That said, it probably won't be merged in until the other TileSet Editor devs have had time to review it and approve it. Merges generally happen

@xDGameStudios
Copy link
Contributor

@akien-mga the merge review will be made for godot 3.2?!

@akien-mga
Copy link
Member

Yes, we are in feature freeze for Godot 3.1.

@akien-mga akien-mga changed the title Add TileProperties,Generalize EditorPropertyResource Add TileProperties, generalize EditorPropertyResource Nov 21, 2018
@willnationsdev
Copy link
Contributor Author

@xDGameStudios don't worry too much. There's a fair chance that this PR could be cherry-picked for 3.1.1.

@TextusGames
Copy link

Thank you for implementing this.
Any chance we can have a new tile mode called "PackedScene" (or built in property for each tile) which main purpes will be to instantiate packed scene? It needs to generate preview of assigned packed scene and replace itself in game or even potentially in editor by instance of packed scene.
Why it is useful:
it will allow to have sets of freaquently used objects and create them in editor as fast and convinient as tile editor allows in oppose of searching them in file manager and drag and droping into scene and than duplicating.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Nov 26, 2018

@TextusGames Yeah, there is an existing Issue for that (#9180). You'd have to create a contextual editor for TileMap so that it could see a list of placeable scenes/scripts and then have it add those. The difficulty would be where to add them. If you started placing the scenes/nodes as children of the TileMap, and you also have multiple TileMaps in your game, it would become almost impossible to properly keep track of which objects exist where when looking at a larger world map on a macro scale (instead of, "give me all of the Goblin scenes that are children of my Goblins node" ($Goblins.get_children()), it would have to be "find all of the TileMap nodes, scan through all of their children at runtime, perform an if-check on each of them to see if they are a Goblin script instance, and if so, return them". The logic can quickly become much more complicated.

In some cases, this distribution may be desired (maybe each TileMap is a self-contained level all on its own for a small-scale game) and it could work very well. In other cases though, with multiple TileMaps for different world layers, multiple configurations of scene/node placement options, etc., it wouldn't be ideal.

I think the best way to handle it would be to use a new Resource that configures what things can be instanced by a TileMap in addition to tiles (a separate list entirely from the currently selected tileset), and then pair each option with a NodePath that specifies where to add the selected node to the scene tree, even if its global position is determined by the TileMap. If undefined, then it could just default to the TileMap itself, and if you wished to re-use a configuration for multiple TileMaps, that would be easy enough.

Edit: I think, with this ^ implemented, you'd start having usability that begins to approximate GameMaker: Studio in terms of tile-based 2D games.

@MarianoGnu
Copy link
Contributor

You can close #9180 adding a virtual function to tile's script to be called on ready pass, something like:

export (PackedScene) var custom

func _setup_tile(p_tilemap, p_worldpos):
    var scn =custom.instance()
    p_tilemap.add_node(scn)
    scn.global_position = p_worldpos

@MarianoGnu
Copy link
Contributor

MarianoGnu commented Nov 27, 2018

Or even a property on TileProperties called "instance_on_startup" and instance it yourself if it exists, then you dont rely on the user to write it himself (because they would probably not know how to do this if dont find a guide or tutorial)
Edit: this could cause a ciclic dependence thought, even when Godot allows it, if you instance the same scene having the tilemap, you will instance child scenes until you get an stack overflow.

@Hoimar
Copy link

Hoimar commented Apr 9, 2019

What's the status of this? Would love to see this in the next release as it's really much needed...

@xDGameStudios don't worry too much. There's a fair chance that this PR could be cherry-picked for 3.1.1.

... but will it be cherry picked, and if yes, when will 3.1.1 be released?

EDIT: I don't mean to put pressure onto anybody, was just asking out of curiosity because this would be a really nice to have feature, being able to build fully-featured tilemaps as opposed to either totally static ones or hacky workarounds for tiles with special properties etc.

@urothis
Copy link

urothis commented Jun 15, 2019

Any update on this possibly being included in 3.2?

@MarianoGnu
Copy link
Contributor

MarianoGnu commented Jun 16, 2019

@willnationsdev i think this PR have a lot of added value to tilesets and tilemaps if conflics are solved, Do you think you will have time to look at it?

@willnationsdev
Copy link
Contributor Author

I can try to take a look at this in the coming days, although I'm pretty busy. I've been out of the Godot game for a few months due to work stuff taking priority. I'll try to rebase and clean up the PR though.

@luanalatte
Copy link

I would love to have this implemented.

@Varicus
Copy link

Varicus commented Aug 19, 2019

Is there any chance we could get this feature also for GridMap and MeshLibrary?

@willnationsdev
Copy link
Contributor Author

@Varicus

Is there any chance we could get this feature also for GridMap and MeshLibrary?

It would basically need to be re-implemented from scratch in both of those classes. I also know nothing about how those classes work and don't mess around with 3D much myself. Anyone is welcome to check out the changes made in this PR and see how to reproduce them for those types (I don't even think the changes would be all that different, probably). Nothing stopping anyone. ;-) I've got other things I'm working on though.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Oct 4, 2019
@TheOnlyJoey
Copy link

Any update on this?
Was doing a quick prototype of a game, and not being able to use properties for tiles makes the TileMap implementation quite useless to me (since most simple things like having a 'type' for a tile for collision testing are essential...). Currently duplicating everything into sprites to just get the prototype done, but will have to wait further developing until this (or another property implementation for TileSet) gets merged.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Dec 26, 2019

@TheOnlyJoey

I haven't bothered rebasing it until 3.2 is released. The fact that it is targeted for 4.0 is basically the only reason it hasn't been merged yet. It's already been marked as ready for a PR meeting to be discussed. Just a matter of them getting to it (there is a long list). Once it is approved, the feature freeze is lifted, and I rebase it, it will be ready to merge.

If you are familiar with git, and building the Godot source, you could easily enough resolve the conflicts yourself and cherry-pick the commit from my fork's branch.

@willnationsdev willnationsdev force-pushed the tileset branch 2 times, most recently from 77a5751 to 19f696f Compare February 12, 2020 22:01
@thegrumpybuffalo
Copy link

is there a plan to have this change merged? would love to have this functionality!

@PhoenixAran
Copy link

Hoping this gets merged sometime in the future. This would be extremely useful in my current project.

@willnationsdev
Copy link
Contributor Author

@thegrumpybuffalo @PhoenixAran

There are a lot of core refactorings going on right now, so if I rebased the pull request's branch on master as it is now, it would just break with more conflicts later as they continue to do core refactorings. I am waiting until that work is done before I rebase the PR and fix all the errors.

And, because it looks like the clang-format rules have been updated since this was initially submitted, there is now a legion of "changes" in the diff that are completely unrelated to the content of the PR. I'm gonna have to sift through everything manually and figure out what should stay and what shouldn't.

@aaronfranke
Copy link
Member

@willnationsdev The majority of the core refactoring work done by the core devs is finished, so anytime soon would be a good time to rebase this.

@willnationsdev willnationsdev force-pushed the tileset branch 4 times, most recently from b952275 to 7b0582c Compare May 15, 2020 06:19
@willnationsdev
Copy link
Contributor Author

This has all been rebased now, btw.

@abueide
Copy link

abueide commented Jun 16, 2020

tried building this, I crash when I try to add a new single tile to a newly created tileset in a blank project

@willnationsdev
Copy link
Contributor Author

@abueide Thanks, I'll try to debug it as soon as I get a chance.

@willnationsdev
Copy link
Contributor Author

  1. Fixed the bug where creating a tile would trigger a fatal error (wasn't checking first whether a TileProperties resource had even been assigned prior to getting its script property).
  2. Fixed a different bug where the meta_properties Dictionary was incorrectly rendering on the individual Tile's section of properties in the TileSet inspector. It was meant only as a TileSet property (for which it is already there and working).
  3. Fixed yet another different bug where the properties and properties_script properties on the individual tile within the TileSet inspector were rendered as integer types rather than resource types. The portrayal of them in the TileSetContextEditor was already functioning correctly though.

@abueide That issue should now be resolved.

@willnationsdev
Copy link
Contributor Author

Just an FYI for anyone looking at this: chances are, this PR will be closed with the work that groud is doing on the new TileSet/TileMap system. And I've mentioned it to him and he already has thoughts about how to allow properties for tiles (or something to that effect). Idk the details.

@jespereggers
Copy link

Are there any news on this? This is such an important feature still missing in Godot :/

@Calinou
Copy link
Member

Calinou commented Apr 2, 2021

Are there any news on this? This is such an important feature still missing in Godot :/

The TileMap and TileSet rework is happening here: #45278

@willnationsdev
Copy link
Contributor Author

This PR has been completely outdated by the work on the TileMap/TileSet refactor and therefore can be closed.

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.

Expose exported script variables in tileset editor.