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

Fix multiple issues with 'add alternative tile' tileset editor button #102640

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

olanti-p
Copy link
Contributor

@olanti-p olanti-p commented Feb 9, 2025

Fix multiple issues with 'add alternative tile' tileset editor button in "Alternative tiles" section:

  1. Fix button being oversized and not rendered properly on small (texture region size < 12px) tilesets (fixes In tileset editor, attempting to paint terrain over alternative tile instead creates a new alternative tile #99764)
  2. Stop button from interfering with MMB panning
  3. Fix undo not working on tiles created with this button
  4. Stop the button from appearing while painting tile properties

The undo doesn't undo increments in next_alternative_id counter within TileSetAtlasSource, but that's an existing issue that also happens when creating alternative tiles via RMB.

Before/after:
image

@AThousandShips AThousandShips requested a review from a team February 10, 2025 08:48
@AThousandShips AThousandShips added bug topic:editor topic:2d cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Feb 10, 2025
@AThousandShips AThousandShips added this to the 4.4 milestone Feb 10, 2025
@groud
Copy link
Member

groud commented Feb 10, 2025

The commits should likely be squashed, but aside from that, looks good to me.

@olanti-p olanti-p force-pushed the fix-add-alt-tile-button branch from 27ed2fb to 33e1598 Compare February 10, 2025 20:19
void TileSetAtlasSourceEditor::_tile_alternatives_create_button_pressed(const Vector2i p_atlas_coords) {
EditorUndoRedoManager *undo_redo = EditorUndoRedoManager::get_singleton();

// FIXME: doesn't undo changes to `next_alternative_id` counter
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// FIXME: doesn't undo changes to `next_alternative_id` counter
// FIXME: Doesn't undo changes to `next_alternative_id` counter.

It just means that some alternative IDs will be skipped when you undo and make them again, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I mention it because the counter is saved, so a sequence of user actions "create tile"->"undo"->"save" produces an unexpected change in the resource file. Intuitive behavior would be to leave no evidence of undone change.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 12, 2025

Actually there is some error spam when creating new alternate tiles

ERROR: No cached rect for tile coords:(1, 2) alternative_id:5
   at: (C:\godot_source\editor/plugins/tiles/tile_atlas_view.cpp:585)
ERROR: No cached rect for tile coords:(1, 2) alternative_id:5
   at: (C:\godot_source\editor/plugins/tiles/tile_atlas_view.cpp:585)
ERROR: No cached rect for tile coords:(1, 2) alternative_id:5
   at: (C:\godot_source\editor/plugins/tiles/tile_atlas_view.cpp:585)
ERROR: No cached rect for tile coords:(1, 2) alternative_id:6

Probably related to the ID problem you mentioned.

@olanti-p
Copy link
Contributor Author

olanti-p commented Feb 12, 2025

Actually there is some error spam when creating new alternate tiles

That seems to happen whenever an alternative tile is created while painting any kind of property.

I'm not sure how to fix these errors, but I could make it so the button is disabled while "Paint" tab is active, and user can't create alternative tiles while painting properties. Would such solution be fine?

Edit: this would be consistent with how alternative tiles can be created via RMB only in "Setup" and "Select" tabs, but not "Paint" tab.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 12, 2025

@groud ^

@groud
Copy link
Member

groud commented Feb 12, 2025

I'm not sure how to fix these errors, but I could make it so the button is disabled while "Paint" tab is active, and user can't create alternative tiles while painting properties. Would such solution be fine?

Well, in theory, the create alternative button should only be visible in the "setup" tab. That's where you define tiles and their alternatives.

Fix button sizing for small tile sizes.
Stop button from preventing MMB panning.
Support undo operations for the button.
Hide button when "Paint" mode is active.
@olanti-p olanti-p force-pushed the fix-add-alt-tile-button branch from 33e1598 to f389776 Compare February 13, 2025 12:58
@olanti-p
Copy link
Contributor Author

Applied suggested changes, and also stopped the button from appearing in "paint" tab.

Well, in theory, the create alternative button should only be visible in the "setup" tab. That's where you define tiles and their alternatives.

At this moment the UI already allows creating and deleting tiles (both base and alternative) in both "setup" and "select" tabs with RMB, and entire alternative tile config has to be done in "select" tab anyway, so stopping the button from appearing in "select" tab feels unnecessarily restrictive. Although if that's something still wanted, I'll change it.

@groud
Copy link
Member

groud commented Feb 13, 2025

At this moment the UI already allows creating and deleting tiles (both base and alternative) in both "setup" and "select" tabs with RMB, and entire alternative tile config has to be done in "select" tab anyway, so stopping the button from appearing in "select" tab feels unnecessarily restrictive. Although if that's something still wanted, I'll change it.

Well, in the "select" mode I think it kind of make sense to make it a bit more difficult to create tiles, as a missclick could easily happen and would maybe be a bit annoying. You can create alternatives there with right click too (though I think the option to create a new alternative should be available in the dropdown menu of alternative tiles too).

In any case, I don't really have a strong opinion on that, so if others want the button, I guess it makes sense to add it there.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 13, 2025

I tested the updated version and it works in both Setup and Select modes without errors, so it's fine.

@Repiteo Repiteo merged commit 4a08fdc into godotengine:master Feb 14, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 14, 2025

Thanks! Congratulations on your first merged contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:editor topic:2d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In tileset editor, attempting to paint terrain over alternative tile instead creates a new alternative tile
5 participants