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 loss of gdextension on editor startup #98041

Merged

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Oct 10, 2024

The GDExtension files were not processed in EditorFileSystem::_first_scan_process_scripts following optimization in #95678.

Thanks to @NetroScript to pinpoint the problem and find the solution. This PR is based on it's suggested modifications. That's why I added him as Coauthor.

I think your approach to use ResourceLoader::get_recognized_extensions_for_type is the right one. It's sounds like a bug to me that the method returns .res and .tres for GDExtension. I'm not an expert of the GDExtensions but I'm pretty sure .res and .tres are not supported for GDExtensions. If you check into GDExtensionLibraryLoader::parse_gdextension_file, it only supports a ConfigFile format and not a resource file. It does not seems easy to modify ResourceLoader::get_recognized_extensions_for_type to not return .res and .tres for GDExtension and I'm worry about side effects so I think that excluding .res and .tres extensions in _first_scan_filesystem seems the right way to me.

Edited: Adding #97962 in the fixed issue list.
Edited: Adding #98056 in the fixed issue list.

@FireCatMagic
Copy link

This should also fix #97962

@NetroScript
Copy link

Thanks for the fast PR. When I checked it out I also found no way to exclude res and tres. As all resource loaders get iterated to find extensions the ResourceFormatLoaderBinary for example also is iterated, which returns true for all handled types (there in theory it would be possible to add an exclusion from my uneducated observations), which causes res to be added.

@Hilderin
Copy link
Contributor Author

Exactly! We will see what reviewers think about that.

@AThousandShips AThousandShips changed the title Fix lost of gdextension on editor startup Fix loss of gdextension on editor startup Oct 10, 2024
@TCROC
Copy link
Contributor

TCROC commented Oct 10, 2024

This also fixes gdextensions not loading on android: #98056

@prfiredragon prfiredragon mentioned this pull request Oct 11, 2024
@Repiteo Repiteo requested review from a team and removed request for a team October 12, 2024 13:37
@fire
Copy link
Member

fire commented Oct 12, 2024

I think we should rename and attempt to modify ResourceLoader::get_recognized_extensions_for_type to not return .res and .tres for GDExtension

Or at least see that its too difficult

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The code seems fine.
This fixes a few GDExtension regressions, so it would be nice to get it merged soon.

It's sounds like a bug to me that the method returns .res and .tres for GDExtension.

It's not exactly a bug. ResourceFormatLoaderText/Binary handle any type of Resource (because you can save/load everything as .res or .tres), and that's where these extensions come from.

@dsnopek
Copy link
Contributor

dsnopek commented Oct 15, 2024

It's not exactly a bug. ResourceFormatLoaderText/Binary handle any type of Resource

I haven't tested it, but I don't think re-saving a .gdextension file as .tres/.res would result in a working GDExtension, because the GDExtension resource class doesn't keep all its data in registered properties. If there's a way to prevent re-saving as a .tres/.res we should probably do that - although, that's not in scope for this PR.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 15, 2024

It's similar issue to #34080

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

This fixes issue #97900 in my testing!

Skimming the code, it looks good to me, but I don't know EditorFileSystem as well as @Hilderin or @KoBeWi - I trust their judgement :-)

@Hilderin Hilderin force-pushed the fix-lost-gdextensions-on-startup branch from f8f0c7d to fbd1643 Compare October 20, 2024 22:40
@Hilderin Hilderin requested review from a team as code owners October 20, 2024 22:40
@Hilderin
Copy link
Contributor Author

As suggested by @fire, I modified code to prevent ResourceLoader::get_recognized_extensions_for_type returning res and tres extensions for GDExtension. I little if added in ResourceFormatLoaderBinary::get_recognized_extensions_for_type and ResourceFormatLoaderText::get_recognized_extensions_for_type did the trick.

@Repiteo Repiteo merged commit a4ed24a into godotengine:master Oct 21, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 21, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment