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 load subtask not being registered leading to false progress values. #90091

Conversation

ajreckof
Copy link
Member

@ajreckof ajreckof commented Apr 1, 2024

Fixes #90076

I'm clearly not 100% sure about my solution but it works. See #90076 (comment) and the previous comment for a bit more depth on why this is working

@ajreckof ajreckof requested a review from a team as a code owner April 1, 2024 05:58
@AThousandShips AThousandShips added this to the 4.3 milestone Apr 1, 2024
@AlexOtsuka
Copy link
Contributor

AlexOtsuka commented Apr 1, 2024

Hey, I just tested this, and as far as I can tell, it solves all the issues from #90076 while maintaining the improvements from #87711 in elegant fashion.

For added context, here are a few outputs from my testing:

Case 1:

core\io\resource_loader.cpp:251 - p_path: res://Bistro_v5_2/BistroExterior.fbx
core\io\resource_loader.cpp:252 - p_original_path: 
core\io\resource_loader.cpp:253 - original_path: res://Bistro_v5_2/BistroExterior.fbx
core\io\resource_loader.cpp:254 - parent_task_path: res://scene.tscn
core\io\resource_loader.cpp:259 - INSERTED

In the regression, an empty String was being inserted in the sub_tasks HashSet. Here, original_path is set to the correct value and the sub task's progress is tracked properly.

Case 2:

core\io\resource_loader.cpp:251 - p_path: res://.godot/imported/BistroExterior.fbx-2cc48855b7169211933e362848048fa2.scn
core\io\resource_loader.cpp:252 - p_original_path: res://Bistro_v5_2/BistroExterior.fbx
core\io\resource_loader.cpp:253 - original_path: res://Bistro_v5_2/BistroExterior.fbx
core\io\resource_loader.cpp:254 - parent_task_path: res://Bistro_v5_2/BistroExterior.fbx
core\io\resource_loader.cpp:262 - NOT INSERTED

This is the scenario #87711 was meant to fix in the first place. We correctly choose p_original_path over p_path, but inserting it in sub_tasks would result in a stack overflow crash later on due to it sharing its local path with its parent task path. This new PR correctly checks for this and skips it as needed.

Case 3 would be similar to Case 2, but with original_path != parent_task_path. I can't test it now, but this is supposedly what happens in the exported build and it looks to me like that should go through with no issues as well.

Thank you for your contribution and great work!!

@akien-mga akien-mga requested a review from RandomShaper April 3, 2024 07:56
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Asking a few changes. Besides that, the changes make a lot of sense.

core/io/resource_loader.cpp Outdated Show resolved Hide resolved
core/io/resource_loader.cpp Outdated Show resolved Hide resolved
core/io/resource_loader.cpp Outdated Show resolved Hide resolved
@ajreckof ajreckof force-pushed the Fix-load-subtask-not-being-registered-leading-to-false-progress-values- branch from 18f0d03 to ca020ad Compare April 12, 2024 23:42
@akien-mga akien-mga requested a review from RandomShaper April 13, 2024 08:08
@akien-mga akien-mga merged commit 00cc0a3 into godotengine:master Apr 24, 2024
16 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.

ResourceLoader.load_threaded_get_status always returns [0] progress
5 participants