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

Avoid reimporting lightmap textures every getter call #77788

Conversation

gongpha
Copy link
Contributor

@gongpha gongpha commented Jun 2, 2023

Also improves #61861. The _get_light_textures_data function is trying to reimport lightmap textures on every getter call which sounds irritable to me (e.g. when the engine is reading the property via duplicating).

@gongpha gongpha requested a review from a team as a code owner June 2, 2023 22:43
@gongpha gongpha force-pushed the stop-posting-about-(re)importing-assets-whatever-dot-exr branch from 8a63989 to 12f4934 Compare June 3, 2023 12:14
@gongpha gongpha requested a review from a team as a code owner June 3, 2023 12:14
@gongpha gongpha force-pushed the stop-posting-about-(re)importing-assets-whatever-dot-exr branch 2 times, most recently from e69af72 to 5f105f1 Compare June 3, 2023 13:04
@clayjohn clayjohn added this to the 4.x milestone Jun 3, 2023
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 329652b), it works as expected. I no longer get a crash with the MRP.

However, we will need to implement a compatibility handler so that users aren't forced to rebake lightmaps when upgrading. (This can take a long time on complex levels with high quality settings, especially if you have a slow GPU.)

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 25, 2023
@akien-mga
Copy link
Member

This indeed needs some compatibility code added. The old methods should still be registered as compatibility methods for GDExtension, and existing data should ideally be parsed and converted to the new format (which can be done via the _set/_get methods).

scene/3d/lightmap_gi.cpp Outdated Show resolved Hide resolved
scene/3d/lightmap_gi.cpp Outdated Show resolved Hide resolved
@gongpha gongpha force-pushed the stop-posting-about-(re)importing-assets-whatever-dot-exr branch 9 times, most recently from 0718e58 to 865c25a Compare August 17, 2023 18:59
@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Oct 2, 2023
@akien-mga akien-mga requested a review from clayjohn October 17, 2023 11:23
@atirut-w
Copy link
Contributor

I don't know what went wrong during the time I last tested this branch, but it no longer works. It crashes with an out of bound error seemingly when there are more than one lightmapped meshes in a scene.

...
ERROR: FATAL: Index p_index = 1 is out of bounds (size() = 1).
   at: CowData<class Ref<class Image> >::get (C:\Users\atiru\Projects\godot\core/templates/cowdata.h:155)

================================================================
CrashHandlerException: Program crashed
Engine version: Godot Engine v4.2.dev.custom_build (865c25adfc9f9842dde9aaf3162f1b768de57c6c)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] CowData<Ref<Image> >::get (C:\Users\atiru\Projects\godot\core\templates\cowdata.h:155)
[1] Vector<Ref<Image> >::operator[] (C:\Users\atiru\Projects\godot\core\templates\vector.h:96)
[2] LightmapGI::bake (C:\Users\atiru\Projects\godot\scene\3d\lightmap_gi.cpp:1088)
[3] LightmapGIEditorPlugin::_bake_select_file (C:\Users\atiru\Projects\godot\editor\plugins\lightmap_gi_editor_plugin.c)
[4] LightmapGIEditorPlugin::_bake (C:\Users\atiru\Projects\godot\editor\plugins\lightmap_gi_editor_plugin.cpp:113)
...

MRP (don't mind the namings, I reused an old MRP): crash.zip

@gongpha
Copy link
Contributor Author

gongpha commented Oct 22, 2023

This crash issue seems not related to this PR. I only refactored the old porting PR (#61861) and removed unnecessary importing operations. You may create a new issue and fill in details about the issue and MRP.

@atirut-w
Copy link
Contributor

atirut-w commented Oct 22, 2023

This issue does not happen in c5a7462, which is the parent of this PR's branch. That should confirm that this issue is caused by something during all those rebases and forced pushes. The stack trace also points to a file touched by this PR:

[2] LightmapGI::bake (C:\Users\atiru\Projects\godot\scene\3d\lightmap_gi.cpp:1088)

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

See above

@gongpha gongpha force-pushed the stop-posting-about-(re)importing-assets-whatever-dot-exr branch from 865c25a to c165f0c Compare October 22, 2023 09:33
@AThousandShips AThousandShips dismissed their stale review October 22, 2023 09:35

Needs to be verified but should fix the issue

@atirut-w
Copy link
Contributor

atirut-w commented Oct 22, 2023

@AThousandShips I tried out your fix on my end and it still crashes with the exact same log, but seems to have progressed a little more. Before, it only said "Retrieving textures", but it now shows reimport progress of the lightmap.

A finally decided to actually test it with a debugger. Here's what I got:

On line 1088: texture_image->blit_rect(images[i * slices_per_texture + j], Rect2i(0, 0, slice_width, slice_height), Point2i(0, slice_height * j));

images is an array with one member in my case BUT is multiplied and added with other things which resulted in a bogus index of 1.

> images[0]
{reference=0x000001baca91e2b0 {format=FORMAT_RGBH (14) data={[size]=6144 [0]=167 'ง' [1]=48 '0' ...} ...} }
> images[1]
Array index out of bounds.
> i * slices_per_texture + j
1

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Needs further evaluation, ensure that the code is correctly transferred and correct

@atirut-w
Copy link
Contributor

New version works now. Turns out I applied it wrong by also commenting out textures.resize(texture_count);.

@akien-mga akien-mga modified the milestones: 4.3, 4.2 Oct 22, 2023
@atirut-w

This comment was marked as off-topic.

@AThousandShips

This comment was marked as off-topic.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Kobewi confirmed in chat that it looks fine as well. I'm comfortable going ahead with this

@akien-mga akien-mga merged commit bf46ee1 into godotengine:master Oct 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@gongpha gongpha deleted the stop-posting-about-(re)importing-assets-whatever-dot-exr branch November 25, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants