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

Add a separate install step after adding all subsystems #1843

Merged
merged 5 commits into from
Oct 31, 2023

Conversation

imberny
Copy link
Contributor

@imberny imberny commented Oct 29, 2023

Since the subsystems are loaded by parsing the directory, I assume the order might vary by OS (I'm on Linux and could reproduce #1819).

* this "install" method can be safely used by subsystems which rely on other subsystems
* fixes dialogic-godot#1819 which was caused by a subsystem fetching another subsytem not yet in the tree (introduced by dialogic-godot#1718)

Since the subsystems are loaded by parsing the directory, I assume the order might vary by OS (I'm on Linux and could reproduce dialogic-godot#1819).
@imberny
Copy link
Contributor Author

imberny commented Oct 29, 2023

Changed the method's name to post_install to better reflect its usage.

@Jowan-Spooner
Copy link
Collaborator

Interesting. The implementation looks pretty clean. I think the original issue could've been solved by just adding
await get_parent().ready
before the Settings.connect statements in the Text subsystem.

So now I'm torn between the simple and the proper solution...

@coppolaemilio
Copy link
Collaborator

@Jowan-Spooner if you think there's a proper solution to this it should be that. The simple solution now might be a headache in the future

@imberny
Copy link
Contributor Author

imberny commented Oct 30, 2023

I guess it depends how often you expect subsystems to rely on eachother. The original issue (I suspect) is due to different OSes loading the subsystems in different orders, meaning it could work for the developer working on a subsystem, but break for others. The benefit of this post_install step is to make it explicit that this is where you put inter-subsystem code.

But as I am very new to this project it could be that such interdependences ought to be rare and that a simple await would do the trick, as long as people remember to include it.

@Jowan-Spooner
Copy link
Collaborator

Jowan-Spooner commented Oct 30, 2023

No, I agree with Emi that a proper solution (such as this PR provides) is likely the better way. Also to answer the question on the review comment, yes all subsystems can be expected to be of tpye DialogicSubsystem so a static declaration would be good.

Thanks for this work!

@imberny
Copy link
Contributor Author

imberny commented Oct 30, 2023

Ok @Jowan-Spooner, I've added an assert as well as a DialogicSubsystem return type. I think the assert is good here, since we already expect that the script would declare a dialogic member variable and thus shouldn't just be any script.

@Jowan-Spooner
Copy link
Collaborator

@imberny Thank you!

@Jowan-Spooner Jowan-Spooner merged commit 7cfbd1b into dialogic-godot:main Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashes new project on start with "Invalid get index 'Settings'..."
3 participants