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

Include animation frames in tile atlas merge. #77316

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

rakkarage
Copy link
Contributor

@rakkarage rakkarage commented May 21, 2023

dst_rect_wide was the same for each frame so kept getting overwritten in the output image I think?
This adjusts dst_rect_wide based on separation and columns and frames.
Closes: #58342
Thanks.

Before:
Screenshot (89)

After:
Screenshot (88)

@rakkarage
Copy link
Contributor Author

rakkarage commented May 22, 2023

Sorry, I am doing something wrong obviously...
the previous atlas has only 9 and a half tiles (instead of 10) in the second screenshot.
I think I fixed it by only using new frame_coords on subsequent frames (frame > 0 ? frame_coords : Vector2i(0, 0))
Thanks.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master b6ba664), I see something completely different when testing with the MRP you linked in the issue. I'm using the atlas merging tool in TileMapAnimation.tscn's TileMap node in the TileSet bottom panel.

master

Screenshot_20230622_042048-1

This PR

image

@akien-mga akien-mga requested review from KoBeWi and groud June 22, 2023 08:37
@rakkarage
Copy link
Contributor Author

rakkarage commented Jun 22, 2023

again

it only seems to work if animations in dst... i guess it needs the same kinda fixes for when animations in src
or in second slot? idk maybe they just get overwritten unless done last? i need to update new_tile_rect_in_atlas instead of just calculating with it i think
will try to fix for this case etc.

merge

@rakkarage rakkarage force-pushed the tileset_atlas_merge branch 2 times, most recently from c76eed7 to 2e779c2 Compare June 22, 2023 16:38
@rakkarage
Copy link
Contributor Author

rakkarage commented Jun 22, 2023

ya sorry it is taking all the frames at least, but new atlas after merge not have animation setup just single frame... will try to get that working too

edit: thanks. all frames are coped, and now all animation data is copied too... seems to work fine

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 23, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Aug 14, 2023

Editor crashes if the animation has more rows:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.2.dev.custom_build (c495eb5102278a110c14bbffbf833ed436d1594d)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] Object::set (C:\godot_source\core\object\object.cpp:224)
[1] AtlasMergingDialog::_generate_merged (C:\godot_source\editor\plugins\tiles\atlas_merging_dialog.cpp:145)
[2] AtlasMergingDialog::_update_texture (C:\godot_source\editor\plugins\tiles\atlas_merging_dialog.cpp:162)
[3] call_with_variant_args_helper<AtlasMergingDialog> (C:\godot_source\core\variant\binder_common.h:308)
[4] call_with_variant_args<AtlasMergingDialog> (C:\godot_source\core\variant\binder_common.h:418)
[5] CallableCustomMethodPointer<AtlasMergingDialog>::call (C:\godot_source\core\object\callable_method_pointer.h:105)
[6] Callable::callp (C:\godot_source\core\variant\callable.cpp:51)
[7] CallableCustomUnbind::call (C:\godot_source\core\variant\callable_bind.cpp:255)
[8] Callable::callp (C:\godot_source\core\variant\callable.cpp:51)
[9] Object::emit_signalp (C:\godot_source\core\object\object.cpp:1073)
[10] Node::emit_signalp (C:\godot_source\scene\main\node.cpp:3577)
[11] Object::emit_signal<int,bool> (C:\godot_source\core\object\object.h:891)
[12] ItemList::gui_input (C:\godot_source\scene\gui\item_list.cpp:697)
[13] Control::_call_gui_input (C:\godot_source\scene\gui\control.cpp:1806)
[14] Viewport::_gui_call_input (C:\godot_source\scene\main\viewport.cpp:1576)
[15] Viewport::_gui_input_event (C:\godot_source\scene\main\viewport.cpp:1805)
[16] Viewport::push_input (C:\godot_source\scene\main\viewport.cpp:3152)
[17] Window::_window_input (C:\godot_source\scene\main\window.cpp:1525)
[18] call_with_variant_args_helper<Window,Ref<InputEvent> const &,0> (C:\godot_source\core\variant\binder_common.h:303)
[19] call_with_variant_args<Window,Ref<InputEvent> const &> (C:\godot_source\core\variant\binder_common.h:418)
[20] CallableCustomMethodPointer<Window,Ref<InputEvent> const &>::call (C:\godot_source\core\object\callable_method_pointer.h:105)
[21] Callable::callp (C:\godot_source\core\variant\callable.cpp:51)
[22] DisplayServerWindows::_dispatch_input_event (C:\godot_source\platform\windows\display_server_windows.cpp:2617)
[23] DisplayServerWindows::_dispatch_input_events (C:\godot_source\platform\windows\display_server_windows.cpp:2582)
[24] Input::_parse_input_event_impl (C:\godot_source\core\input\input.cpp:723)
[25] Input::flush_buffered_events (C:\godot_source\core\input\input.cpp:980)
[26] DisplayServerWindows::process_events (C:\godot_source\platform\windows\display_server_windows.cpp:2301)
[27] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:1479)
[28] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:182)
[29] _main (C:\godot_source\platform\windows\godot_windows.cpp:204)
[30] main (C:\godot_source\platform\windows\godot_windows.cpp:218)
[31] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:232)
[32] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[33] <couldn't map PC to fn name>
-- END OF BACKTRACE --

In the issue's MRP change columns of the lower animation to 3 and try to merge it.

@rakkarage
Copy link
Contributor Author

rakkarage commented Aug 15, 2023

Thanks for feedback/review.

Another issue I just noticed. Even with no columns some animation frames are a pixel or two offset in output.
Untitled

I cannot replicate the crash, testing with the 4.1.x that I was using at the time? I set the columns of the second animation to 3 and it works but I still get the offset bug. Maybe I should merge / rebase it all up to latest version or idk...
Untitled333

EDIT: I replicated the crash... by merging animation down instead of up. (merge middle animation texture with next texture in list instead of with previous texture) thanks

Looking into fixing this...

@rakkarage
Copy link
Contributor Author

rakkarage commented Aug 20, 2023

fixed the brackets and pixel offset...

atlas_size.x += extra.x;
this code is wrong too... tile coords + pixel coords = gibberish
will fix, sorry should not have pushed/spam when not 100%

Edit: ya still crashing... with animation_columns. I think it is because am failing to account for changes to atlas_size.y when columns causes extra rows...

@rakkarage rakkarage force-pushed the tileset_atlas_merge branch 4 times, most recently from 9904115 to 73bfd0c Compare August 23, 2023 14:43
@rakkarage
Copy link
Contributor Author

rakkarage commented Aug 23, 2023

Fixed the crash (maybe) by dealing with extra rows added by animation columns...
But I think maybe a better solution would be to ignore/remove columns. or add an option?

  • less code, don't even need to deal with atlas_size.y
  • the code already removes margins and separation when merging to new atlas... so removing columns is similar.
  • efficient use of space. the rest of the expanded rows are currently wasted.

Screenshot 2023-08-23 102039
Screenshot 2023-08-23 102905

Edit: Also will prob need to deal better / fix animation_separation and get rid of that or account for it in atlas size too. Separation is removed so animation separation should be too right?

@akien-mga akien-mga merged commit 2d42357 into godotengine:master Aug 28, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

The new TileSet Atlas Merging Tool does not include Animations
7 participants