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

Rework modifying tile source ID #79419

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 13, 2023

Part of godotengine/godot-proposals#7177

This PR unexposed atlas/scene source ID property from the inspector:
image

It's an advanced feature and providing it for inexperienced users may, in worst case, cause data loss which might be not easy to recover (see the proposal). Changing ID is still possible via scripting. I wonder if it should be documented 🤔

@groud
Copy link
Member

groud commented Jul 13, 2023

Hmm, I disagree on removing it, it's needed to be accessible somewhere in the editor. This is needed in case you want to replace an atlas by another or something like that, so it has some uses in the editor. But yeah, I understand it's dangerous to expose it so easily.

What I would suggest:

  • Remove the ID from the source list (it's not that useful)
  • Still display the ID in the inspector, but not editable (disabled state or with a label).
  • Add a button to change the ID, that would popup a window with a confirmation of the new ID and a warning about the consequences.

Ideally, I believe this should be done with a custom property editor. (like with the ID value and an "edit" button)

@MewPurPur
Copy link
Contributor

I was experimenting with a lot of tilesets until finally settling down on my approach. After my failed attempts, the IDs were something like 3, 5, and 8. I'm very glad I could just put the tiles source in ID 1, decorations source in ID 2, and interactables in ID 3. Would've been quite annoying to use in code otherwise.

I second gr0ud, doesn't seem wise to remove this fully.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 13, 2023

What I meant by "possible via scripting" is that you can use a simple EditorScript to re-assign your IDs:

func _run():
	var tiles = load("res://tileset.tres")
	tiles.get_source(old_id).set_id(new_id)
	ResourceSaver.save(tiles)

But I can look into making the suggested changes .-.

@MewPurPur
Copy link
Contributor

MewPurPur commented Jul 13, 2023

EditorScript isn't something that tilemap users who would need this necessarily know how to use.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 14, 2023

godot.windows.editor.dev.x86_64_ivASbN7u6e.mp4

I tried to make the dialog not so ridiculously wide, but failed.

EditorScript isn't something that tilemap users who would need this necessarily know how to use.

That's why I mentioned that we could document it.

@Calinou
Copy link
Member

Calinou commented Jul 14, 2023

That's why I mentioned that we could document it.

Running code in the editor makes a brief mention of EditorScript, but it should be amended to show a full concrete example (e.g. automatically multiplying the range of all lights in the scene).

@KoBeWi KoBeWi force-pushed the no_ID_for_you branch 2 times, most recently from b55c8e7 to 9e20924 Compare July 19, 2023 11:49
@bgie
Copy link
Contributor

bgie commented Jul 25, 2023

Like the current commit and the warning, instead of 'unexposing' like in the original title.

You need the source id for set_cell when doing procedural generation of maps.

@YuriSizov
Copy link
Contributor

Ah, needs a rebase now.

@YuriSizov YuriSizov merged commit 6731acc into godotengine:master Aug 1, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov YuriSizov changed the title Unexpose tile source ID Rework modifying tile source ID Aug 1, 2023
@KoBeWi KoBeWi deleted the no_ID_for_you branch August 1, 2023 15:40
@KoBeWi KoBeWi mentioned this pull request Mar 28, 2024
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.

6 participants