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

Implement scenes tiles in TileMaps #48812

Merged
merged 1 commit into from
May 20, 2021

Conversation

groud
Copy link
Member

@groud groud commented May 18, 2021

Peek 18-05-2021 13-28

Implements a TileSetScenesCollectionSource for TileSets. Each tile in this TileSet source has a scene that will be instantiated as an internal child of the TileMap node.

I also implemented a "display placeholder" property that allow displaying a placeholder at the position of the tile in case a scene is not visible.

Also, I moved TileSetAtlasSource::INVALID_* to TileSetSource::INVALID_*

@groud groud added this to the 4.0 milestone May 18, 2021
@groud groud requested a review from a team as a code owner May 18, 2021 14:56
@groud groud requested a review from a team as a code owner May 18, 2021 15:02
scene/resources/tile_set.cpp Outdated Show resolved Hide resolved
doc/classes/TileSetScenesCollectionSource.xml Show resolved Hide resolved
editor/plugins/tiles/tile_map_editor.cpp Outdated Show resolved Hide resolved
scene/resources/tile_set.h Outdated Show resolved Hide resolved
scene/resources/tile_set.h Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented May 19, 2021

  • Not sure how you should prevent that, but it's possible to draw the scene that you are currently editing, resulting in cyclic dependency. Although it just won't save, so maybe it's handled correctly.
  • I managed to make the editor crash once, not sure how:
CrashHandlerException: Program crashed
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[0] Ref<SceneState>::operator-> (C:\godot_source\core\object\reference.h:109)
[1] PackedScene::instance (C:\godot_source\scene\resources\packed_scene.cpp:1626)
[2] TileSetPluginScenesCollections::update_dirty_quadrants (C:\godot_source\scene\resources\tile_set.cpp:4484)
[3] TileMap::update_dirty_quadrants (C:\godot_source\scene\2d\tile_map.cpp:373)
[4] call_with_variant_args_helper<TileMap> (C:\godot_source\core\variant\binder_common.h:214)
[5] call_with_variant_args_dv<TileMap> (C:\godot_source\core\variant\binder_common.h:357)
[6] MethodBindT<TileMap>::call (C:\godot_source\core\object\method_bind.h:285)
[7] Object::call (C:\godot_source\core\object\object.cpp:784)
[8] Callable::call (C:\godot_source\core\variant\callable.cpp:53)
[9] MessageQueue::_call_function (C:\godot_source\core\object\message_queue.cpp:259)
[10] MessageQueue::flush (C:\godot_source\core\object\message_queue.cpp:306)
[11] SceneTree::physics_process (C:\godot_source\scene\main\scene_tree.cpp:415)
[12] Main::iteration (C:\godot_source\main\main.cpp:2479)
[13] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:622)
[14] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:163)
[15] _main (C:\godot_source\platform\windows\godot_windows.cpp:185)
[16] main (C:\godot_source\platform\windows\godot_windows.cpp:199)
[17] __scrt_common_main_seh (D:\a01\_work\26\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[18] BaseThreadInitThunk
-- END OF BACKTRACE --
  • Scene tiles are always centered. They either should have configurable offset or at least use the position of instance's root node.
  • I get this bug for some reason:
    le Bug

@groud
Copy link
Member Author

groud commented May 19, 2021

Not sure how you should prevent that, but it's possible to draw the scene that you are currently editing, resulting in cyclic dependency. Although it just won't save, so maybe it's handled correctly.

I don't think I can do anything about it. TileSet is a resource, so it can't be strictly associated to the scene it is in. So well... as soon as it does not crashes I believe this is fine, as the engine does not checks for cyclical dependencies in general.

I managed to make the editor crash once, not sure how:

It happens when trying to paint a scene with an invalid scene. It should be fixed now.

Scene tiles are always centered. They either should have configurable offset or at least use the position of instance's root node.

Fixed.

I get this bug for some reason:

Fixed too, I screwed things up when rebasing... ^^

@groud
Copy link
Member Author

groud commented May 19, 2021

I have no clue why the Android build fails and how to solve it. That's annoying... :/

@groud groud requested a review from a team as a code owner May 19, 2021 16:14
akien-mga added a commit to akien-mga/godot that referenced this pull request May 19, 2021
We found that this flag causes this error on PR godotengine#48812 which does not add any
fancy inline assembly:
```
/tmp/tile_set-ce236a.s: Assembler messages:
/tmp/tile_set-ce236a.s:34676: Error: selected processor does not support `bfc x0,godotengine#32,godotengine#32'
clang++: error: assembler command failed with exit code 1 (use -v to see invocation)
```

That flag is mentioned in various errors related to assembler failures on
arm64v8 with Clang from the Android NDK.

It was added in Godot in godotengine#6958 when migrating from GCC to Clang, and is indeed
referenced in the NDK's Clang migration guide:
https://android.googlesource.com/platform/ndk/+/master/docs/ClangMigration.md

> Especially for ARM and ARM64, Clang is much stricter about assembler rules
> than GCC/GAS. Use `-fno-integrated-as` if Clang reports errors in inline
> assembly or assembly files that you don't wish to modernize.

We don't get those errors nowadays so it seems the flag is no longer needed.
akien-mga added a commit that referenced this pull request May 19, 2021
We found that this flag causes this error on PR #48812 which does not add any
fancy inline assembly:
```
/tmp/tile_set-ce236a.s: Assembler messages:
/tmp/tile_set-ce236a.s:34676: Error: selected processor does not support `bfc x0,#32,#32'
clang++: error: assembler command failed with exit code 1 (use -v to see invocation)
```

That flag is mentioned in various errors related to assembler failures on
arm64v8 with Clang from the Android NDK.

It was added in Godot in #6958 when migrating from GCC to Clang, and is indeed
referenced in the NDK's Clang migration guide:
https://android.googlesource.com/platform/ndk/+/master/docs/ClangMigration.md

> Especially for ARM and ARM64, Clang is much stricter about assembler rules
> than GCC/GAS. Use `-fno-integrated-as` if Clang reports errors in inline
> assembly or assembly files that you don't wish to modernize.

We don't get those errors nowadays so it seems the flag is no longer needed.

(cherry picked from commit 23f7c75)
akien-mga added a commit that referenced this pull request May 19, 2021
We found that this flag causes this error on PR #48812 which does not add any
fancy inline assembly:
```
/tmp/tile_set-ce236a.s: Assembler messages:
/tmp/tile_set-ce236a.s:34676: Error: selected processor does not support `bfc x0,#32,#32'
clang++: error: assembler command failed with exit code 1 (use -v to see invocation)
```

That flag is mentioned in various errors related to assembler failures on
arm64v8 with Clang from the Android NDK.

It was added in Godot in #6958 when migrating from GCC to Clang, and is indeed
referenced in the NDK's Clang migration guide:
https://android.googlesource.com/platform/ndk/+/master/docs/ClangMigration.md

> Especially for ARM and ARM64, Clang is much stricter about assembler rules
> than GCC/GAS. Use `-fno-integrated-as` if Clang reports errors in inline
> assembly or assembly files that you don't wish to modernize.

We don't get those errors nowadays so it seems the flag is no longer needed.

(cherry picked from commit 23f7c75)
@KoBeWi
Copy link
Member

KoBeWi commented May 19, 2021

Seems like it's possible to draw 2 scenes at one when you click between tiles:
Bug2
But only horizontally.

EDIT:
Everything else seems ok.

@groud
Copy link
Member Author

groud commented May 20, 2021

Seems like it's possible to draw 2 scenes at one when you click between tiles:

Oh oh, that was a tricky one. My _draw_line function took one Vector2i and one Vector2 as arguments, while they should have been both Vector2. This caused very small differences between the two computed ends of a line, even if, in that case, the end and the beginning of the line should were supposed to have the same value.

Anyway, that's fixed now, thanks!

@akien-mga akien-mga requested a review from Calinou May 20, 2021 12:29
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I didn't test, but since @KoBeWi did and code seems fine, should be good to go.

@akien-mga akien-mga merged commit a6a75e2 into godotengine:master May 20, 2021
@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.

4 participants