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.

Sign up for GitHub

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

Jump to bottom

Fix building without tools enabled #281

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Fix building without tools enabled #281

wants to merge 32 commits into from

Conversation

Beliar83
Copy link
Contributor

Disable code that references editor specific things when TOOLS_ENABLED is not defined.
This allows building of export templates.

AndreaCatania
AndreaCatania requested changes Jan 16, 2022
View reviewed changes
Copy link
Member

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

This PR looks really nice, and it's super helpful. I think we just need to find the proper way of fetching the scripts in production, if EditorFileSystem::get_singleton() is not available, otherwise we will not be able to properly execute the scripted components.

modules/godot/editor_plugins/editor_world_ecs.cpp Outdated Show resolved Hide resolved
modules/godot/editor_plugins/editor_world_ecs.cpp Outdated
@@ -1352,3 +1357,4 @@ void WorldECSEditorPlugin::make_visible(bool p_visible) {
ecs_editor->set_world_ecs(nullptr);
}
}
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, a new line is missing here, the CI will fail

modules/godot/nodes/ecs_world.cpp Outdated
@@ -432,6 +432,7 @@ TypedArray<String> WorldECS::get_configuration_warnings() const {
warnings.push_back(TTR("The selected pipeline `") + active_pipeline + TTR("` doesn't exists in this WorldECS."));
}

#ifdef TOOLS_ENABLED
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if wrap the entire function with ifdef would make more sense instead, what do you think?

modules/godot/nodes/script_ecs.cpp Show resolved Hide resolved
modules/godot/nodes/script_ecs.cpp Show resolved Hide resolved
modules/godot/nodes/script_ecs.cpp Outdated Show resolved Hide resolved
modules/godot/nodes/script_ecs.cpp Show resolved Hide resolved
@Beliar83
Copy link
Contributor Author

After some more thinking I think many errors would already be gone if the files in the editor_plugins are not included with tools disabled. This can already be done in the SCsub. Do you see any problem with that?

@Beliar83
Copy link
Contributor Author

@AndreaCatania What exactly would the problem be with scripted components?

@AndreaCatania
Copy link
Member

After some more thinking I think many errors would already be gone if the files in the editor_plugins are not included with tools disabled. This can already be done in the SCsub. Do you see any problem with that?

I think that would make a lot of sense doing that TBH.

What exactly would the problem be with scripted components?

At startup the scripted component need to be fetched, and registered to godex to be used. It uses the function ScriptEcs::reload_scripts() to do that, so we need a way to preserve the ability to load the scripts even in production.

@Beliar83
Copy link
Contributor Author

Beliar83 commented Jan 16, 2022 •
edited
Loading

At startup the scripted component need to be fetched, and registered to godex to be used. It uses the function ScriptEcs::reload_scripts() to do that, so we need a way to preserve the ability to load the scripts even in production.

I see, okay.

@AndreaCatania
Copy link
Member

It looks nice now, the only issue is find an alternative way of fetching the scripts in production. I think it's enough see how GDScript handle that, since it should work in the same way. I'll try to look into that, and ask to the godot team, feel free to do it too.

@Beliar83 Beliar83 force-pushed the export_templates branch from 9360eb2 to b89d2b7 Compare May 6, 2022 05:44
@Beliar83
Update compatibility for 4.0 beta3 (35cfaafda8073f700c9d2fe42a43d3d81…
2fd59b7 
…eaaea67)
@Beliar83
Update compatibility to 889868cbbc8beac74d5f49f9b7ef41efc4ae7d5a
c5bba89
@Beliar83
Ignore script name when checking properties
b99963a
@Beliar83
Temporarily disable registering scripts when running a project to fix…
f8db5b6 
… crash
@Beliar83
Fix loading of components from C#
f8bf505 
Remove VisualScript checking, as it was discontinued
@Beliar83
Update compatibility
1a200e4
@Beliar83
Update compatibility
6d2a012
@Beliar83
Move register_runtime_scripts to NOTIFICATION_POSTINITIALIZE
f3a5fd2
@Beliar83
Call register_runtime_scripts in NOTIFICATION_READY as well.
1412b3c
@Beliar83
Fix formatting
5291ad1
@Beliar83
Compatibility changes
9ab9ed5 
Handle WorldECSEditorPlugin::edit being called with nullptr
Move adding Components3DGizmoPlugin to _editor_init
Reset default properties when destructing ScriptEcs
@Beliar83
Change godot version to check out
0069914
@Beliar83
Adjust target
27d873b
@Beliar83
Update patch
9c282c2
@Beliar83
Fix g++ warnings
abb7ed4
@Beliar83
Fixing builds
dc228d8 
Remove explicit -j option and let the godot build system automatically find the best
@Beliar83
Fix formatting
3b7098e
@Beliar83
Fix godot tests
b8514f0
@Beliar83
Fix tests
e0c8e08
@Beliar83
Preload scripts for registering system and component names
eac4c95
@Beliar83
Wait for static checks
d62c5f2
@Beliar83
Copy link
Contributor Author

@AndreaCatania This should fix the problem with not being able to use EditorFileSystem. I also based it on the changes in #304 , so you might want to check that first.

@Beliar83 Beliar83 force-pushed the export_templates branch 3 times, most recently from 39e8f43 to dc730aa Compare February 23, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreaCatania AndreaCatania Awaiting requested review from AndreaCatania

Assignees
No one assigned
Labels
Hero! topic:editor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Beliar83 @AndreaCatania