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 MeshLibary previews #80787

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Aug 19, 2023

Fixes #76429
Depends on #80782

This PR adds a new property to MeshLibrary items: use_custom_preview. If disabled (default), the preview will not be saved with the resource. Instead the editor will auto-generate the previews every time the library is loaded and when it changes. This makes MeshLibrary resources much smaller, as the preview textures no longer get embedded 😬

HxdZn1tdeU.mp4

It's not perfect, because when you change the mesh's material, not the mesh itself, the change will not be detected. You can however clear the preview manually and it will get auto-generated again.

godot.windows.editor.dev.x86_64_prlIl4xT81.mp4

Using set_item_preview() (bound) will automatically enable the use_custom_preview. Also I added an option that preserves the previous behavior of embedding previews:

image

While this PR preserves API compatibility, if someone had custom previews in their MeshLibrary they need to enable use_custom_preview property for each item, otherwise the previews will be lost.

Also this PR needs testing with large MeshLibrary with complex meshes. I reworked preview generation to be much faster, but I don't know if it's sufficiently fast.

@akien-mga
Copy link
Member

The UX of these file dialogs with checkboxes at the bottom is really subpar.

We should maybe see if we can reach consensus on whether #79313 is a good approach (Blender-like), and use this here too and in the project export dialog.

Not blocking for this PR though, we can improve all these dialogs later on.

@geowarin
Copy link
Contributor

After a bit of testing, this feels really good on small mesh libraries 👍


However, when selecting the scene where my GridMaps are used, after the initial opening, I get spammed with this type of errors:

./servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:728 - Condition "!mesh" is true.

It does not seem to affect anything however. Just curious, I'm pretty sure I was not getting those error before.


The speedup improvement feels almost magical, I wonder how you pulled this off?
I'm under the impression that EditorInterface.make_mesh_previews() did not benefit from the speedup so I'm assuming that improving the code in the MeshLibrary was enough.

The size diminution of the MeshLibraries is also very nice!

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 20, 2023

The speedup improvement feels almost magical, I wonder how you pulled this off?

EditorInterface's make_mesh_previews() will always create and then destroy viewport with a whole environment to render the previews. I extracted the rendering part into a new method and the made GridMapEditorPlugin cache its own environment, so making the icons now only involves rendering meshes in a previously created viewport.

@geowarin
Copy link
Contributor

geowarin commented Aug 21, 2023

I might have found a regression with the preview:

I have a MeshLibrary generated from this (messy) scene, where all the meshes are piled onto one another:

image

With the new on-the-fly preview mode I get this result:

image

Whereas, with the baked previews, I get this:

image

EDIT: The bug disappeared after reopening the project... 🤔

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 21, 2023

However, when selecting the scene where my GridMaps are used, after the initial opening, I get spammed with this type of errors:

Can't reproduce it. Can you share your scene?

I have a MeshLibrary generated from this (messy) scene, where all the meshes are piled onto one another:

Can you share the problematic mesh?

@geowarin
Copy link
Contributor

geowarin commented Aug 21, 2023

Ok I can reliably reproduce this by:

  • having the main scene where the GripMap using the MeshLibrary open
  • Going to the scene with the meshes, Export as > MeshLibrary
  • Unchecking "merge with existing"
  • Override the previously generated mesh library (floors2.tres)
  • Going back to the main scene => previews are messed up
  • Reload the scene => previews are fine again

By doing this, I also get the error message:

ERROR: Condition "!mesh" is true. Returning: 0
   at: mesh_get_surface_count (./servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:581)

Test project (it's a big chuncky sorry, I did not clean it up)

repro.zip

In this project, there are two meshlibs: floors.tres (with embedded previews) and floors2.tres with no custom previews

EDIT: I also got a segfault while playing with this but I forgot to make a dump and I can't reproduce it right now

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 21, 2023

EDIT: I also got a segfault while playing with this but I forgot to make a dump and I can't reproduce it right now

I think I know what caused it. I'll make a fix.

@KoBeWi KoBeWi force-pushed the wind_of_changes_sweeps_away_your_mesh_previews_into_oblivion branch 2 times, most recently from 3c36cd3 to 76f0df6 Compare August 21, 2023 11:15
@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 21, 2023

I can reproduce the preview bug, but no idea why it happens.

As for the crash, it might happen when you have GridMap selected and close the scene tab. But it seems to have existed before my PR.
@geowarin Can you check if you can reproduce the crash by closing the scene with GridMap active? It looks system-specific, I only get an error printed that should be a crash. EDIT2: #80849 should fix it.

EDIT:
btw your project's MeshLibrary is somewhat big and the previews seem to be loaded sequentially instead of all at once, so I think it won't cause performance problems.

@KoBeWi KoBeWi force-pushed the wind_of_changes_sweeps_away_your_mesh_previews_into_oblivion branch from 76f0df6 to 7f9af2c Compare August 21, 2023 12:17
@geowarin
Copy link
Contributor

geowarin commented Aug 21, 2023

Sorry no luck reproducing the segfault...

I'm getting another bug:

image

The first time I select the GridMap, the panel is filled with empty textures.

Probably from here?

It disappears when I deselect, then re-select it. It does not seem that this happens on the embedded previews.

EDIT: Call stack suggest there is a loop

image

updatePalette() -> setItemPreview() -> emit_changed() -> update_palette()

It does finish though.

When I select one of the undefined textures, it seems to paint the first item of the mesh library (id 0 probably).

It only happens once when you open the scene with the GridMap being unselected (so you have to save the scene with the root selected, for instance) and then select the GridMap.

@KoBeWi KoBeWi force-pushed the wind_of_changes_sweeps_away_your_mesh_previews_into_oblivion branch from 7f9af2c to 1dfb62d Compare August 27, 2023 15:18
@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 27, 2023

@geowarin Can you test again? I think I fixed the crash/empty items bug (they were related).

@@ -1163,10 +1181,6 @@ void GridMapEditor::_item_selected_cbk(int idx) {
}

void GridMapEditor::_floor_changed(float p_value) {
if (updating) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I repurposed this bool for something else. It wasn't needed here anyway.

@geowarin
Copy link
Contributor

geowarin commented Aug 28, 2023

Got a backtrace for the segfault, it's exactly what you said it was:

#0  0x0000000000000060 in ?? ()
#1  0x000055555abbaed4 in GridMapEditor::edit (this=0x555570e7c650, p_gridmap=0x0) at modules/gridmap/editor/grid_map_editor_plugin.cpp:949
#2  0x000055555abc2f7f in GridMapEditorPlugin::edit (this=0x555570e7bf30, p_object=0x0) at modules/gridmap/editor/grid_map_editor_plugin.cpp:1510
#3  0x000055555b637a6d in EditorNode::_plugin_over_edit (this=0x5555625b2f40, p_plugin=0x555570e7bf30, p_object=0x0) at ./editor/editor_node.cpp:779
#4  0x000055555b641e9e in EditorNode::hide_unused_editors (this=0x5555625b2f40, p_editing_owner=0x555567a7ffd0) at ./editor/editor_node.cpp:2143
#5  0x000055555b9213e9 in SceneTreeDock::_push_item (this=0x555567a7ffd0, p_object=0x0) at ./editor/scene_tree_dock.cpp:1496
#6  0x000055555b9275c9 in SceneTreeDock::_selection_changed (this=0x555567a7ffd0) at ./editor/scene_tree_dock.cpp:2290
#7  0x000055555b9e896a in call_with_variant_args_helper<SceneTreeDock>(SceneTreeDock*, void (SceneTreeDock::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (p_instance=0x555567a7ffd0, p_method=(void (SceneTreeDock::*)(SceneTreeDock * const)) 0x55555b927510 <SceneTreeDock::_selection_changed()>, p_args=0x0, r_error=...) at ./core/variant/binder_common.h:303
#8  0x000055555b9e27f6 in call_with_variant_args<SceneTreeDock> (p_instance=0x555567a7ffd0, p_method=(void (SceneTreeDock::*)(SceneTreeDock * const)) 0x55555b927510 <SceneTreeDock::_selection_changed()>, p_args=0x0, p_argcount=0, r_error=...) at ./core/variant/binder_common.h:417
#9  0x000055555b9c412d in CallableCustomMethodPointer<SceneTreeDock>::call (this=0x555567b978d0, p_arguments=0x0, p_argcount=0, r_return_value=..., r_call_error=...) at ./core/object/callable_method_pointer.h:104
#10 0x000055555e0a8f32 in Callable::callp (this=0x55557db23200, p_arguments=0x0, p_argcount=0, r_return_value=..., r_call_error=...) at ./core/variant/callable.cpp:50
#11 0x000055555e412c8a in Object::emit_signalp (this=0x55556115ab50, p_name=..., p_args=0x0, p_argcount=0) at ./core/object/object.cpp:1072
#12 0x000055555acdd00e in Object::emit_signal<>(StringName const&) (this=0x55556115ab50, p_name=...) at ./core/object/object.h:891
#13 0x000055555b45d4a7 in EditorSelection::_emit_change (this=0x55556115ab50) at ./editor/editor_data.cpp:1292
#14 0x000055555a3f8386 in call_with_variant_args_helper<__UnexistingClass>(__UnexistingClass*, void (__UnexistingClass::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (p_instance=0x55556115ab50, p_method=(void (__UnexistingClass::*)(__UnexistingClass * const)) 0x55555b45d470 <EditorSelection::_emit_change()>, p_args=0x7fffffffb400, r_error=...) at ./core/variant/binder_common.h:303
#15 0x000055555a3f43f6 in call_with_variant_args_dv<__UnexistingClass> (p_instance=0x55556115ab50, p_method=(void (__UnexistingClass::*)(__UnexistingClass * const)) 0x55555b45d470 <EditorSelection::_emit_change()>, p_args=0x0, p_argcount=0, r_error=..., default_values=...) at ./core/variant/binder_common.h:450
#16 0x000055555a3ef198 in MethodBindT<>::call(Object*, Variant const**, int, Callable::CallError&) const (this=0x555562369b40, p_object=0x55556115ab50, p_args=0x0, p_arg_count=0, r_error=...) at ./core/object/method_bind.h:331
#17 0x000055555e410cef in Object::callp (this=0x55556115ab50, p_method=..., p_args=0x0, p_argcount=0, r_error=...) at ./core/object/object.cpp:741
#18 0x000055555e0a9220 in Callable::callp (this=0x5555623f3258, p_arguments=0x0, p_argcount=0, r_return_value=..., r_call_error=...) at ./core/variant/callable.cpp:62
#19 0x000055555e40b0aa in CallQueue::_call_function (this=0x55555f5fa250, p_callable=..., p_args=0x5555623f3270, p_argcount=0, p_show_error=false) at ./core/object/message_queue.cpp:219
#20 0x000055555e40b87f in CallQueue::flush (this=0x55555f5fa250) at ./core/object/message_queue.cpp:324
#21 0x000055555c404aa0 in SceneTree::physics_process (this=0x5555625451a0, p_time=0.016666666666666666) at ./scene/main/scene_tree.cpp:471
#22 0x000055555a1e9f98 in Main::iteration () at main/main.cpp:3463
#23 0x000055555a15e4f0 in OS_LinuxBSD::run (this=0x7fffffffb8e0) at platform/linuxbsd/os_linuxbsd.cpp:933
#24 0x000055555a156704 in main (argc=4, argv=0x7fffffffbea8) at platform/linuxbsd/godot_linuxbsd.cpp:74
#25 0x00007ffff5227cd0 in ?? () from /usr/lib/libc.so.6
#26 0x00007ffff5227d8a in __libc_start_main () from /usr/lib/libc.so.6
#27 0x000055555a156495 in _start ()

GridMapEditor::edit(GridMap *p_gridmap) with p_gridmap being NULL

@geowarin
Copy link
Contributor

Everything seems to be working fine!

I'm still getting this harmless error after re-exporting a MeshLibrary and selecting the associated GridMap node for the first time but it is no longer spammed (only happens once):

./servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:581 - Condition "!mesh" is true. Returning: 0

Otherwise, previews are working fine.
Not saving the previews in the MeshLibrary is a nice size improvement: 2.3MiB vs 4.1MiB in the example I provided.

The tradeoff seems fine to me. On this example, generating the previews is near instantaneous and it only happens once, when loading the MeshLibrary.

People using very big mesh libraries still have the option to bake the previews.

My only concern is what happens when you are working on a MeshLibrary generated with version 4.1.
I have to test this further but if I understand it correctly, the checkbox for custom preview will be unchecked so they will be generating previews dynamically without seeing the size improvement.

For people to benefit from it, they will have to re-export their MeshLibraries is that correct?

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 28, 2023

MeshLibraries from 4.1 have their previews stored inside the library and they will stay.
When you export the scene as MeshLibrary, it replaces the resource, so if you don't enable baking, your meshlib will be replaced with a one without previews. I think Merge With Existing will keep previews, but I didn't test nor check how it's implemented, so not sure.

@eimfach
Copy link

eimfach commented Sep 16, 2023

Would it be possible to add an option to show the preview as only the flat texture of the material ? Like I have planes in 3D spaces placed as walls and floors and my previews are cut off, thus not being able to properly distinguish them from another..

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 16, 2023

That's out of scope of this PR.

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.

Meshinstance Preview Picture and GridMap Preview Picture does not change when you change a texture
4 participants