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

Update compatibility to 4.0-stable #304

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

Conversation

Beliar83
Copy link
Contributor

@Beliar83 Beliar83 commented Oct 31, 2022

This makes godex compile with commit 92bee43adba8d2401ef40e2480e53087bcb1eaf1, which is the one that was used to build the stable reelase of Godot 4.0.

I am not 100% if the function I wrote to replace remove_and_skip is working correctly. I did some quick tests and it looks okay, but maybe someone with a deeper knowledge of godex should double check.

@Beliar83 Beliar83 changed the title Update compatibility for 4.0 beta3 Update compatibility for 4.0 rc1 Feb 11, 2023
@Beliar83
Copy link
Contributor Author

I have added changes for compatibility for up to 4.0 rc1.
I also added a fix for register_runtime_scripts that does not need a godot change.

Handle WorldECSEditorPlugin::edit being called with nullptr
Move adding Components3DGizmoPlugin to _editor_init
Reset default properties when destructing ScriptEcs
Remove explicit -j option and let the godot build system automatically find the best
@Beliar83 Beliar83 changed the title Update compatibility for 4.0 rc3 Update compatibility to 4.0-stable Mar 1, 2023
Copy link

@RevoluPowered RevoluPowered left a comment

Choose a reason for hiding this comment

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

Minor comments left if they're irrelevant feel free to ignore :) I didn't see anything scary

info->property_map.resize(p_properties.size());
info->properties.resize(p_properties.size());
info->defaults.resize(p_properties.size());

// Validate and initialize the parameters.
for (uint32_t i = 0; i < p_properties.size(); i += 1) {
if (

Choose a reason for hiding this comment

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

extra whitespace

@@ -332,6 +338,13 @@ String Component::validate_script(Ref<Script> p_script) {
List<PropertyInfo> properties;
p_script->get_script_property_list(&properties);
for (List<PropertyInfo>::Element *e = properties.front(); e; e = e->next()) {
if (

Choose a reason for hiding this comment

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

extra whitespace

// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Gizmos
Components3DGizmoPlugin::get_singleton()->add_component_gizmo(memnew(TransformComponentGizmo));
Components3DGizmoPlugin::get_singleton()->add_component_gizmo(memnew(MeshComponentGizmo));

} else {
// Load the Scripted Components/Databags/Systems
ScriptEcs::get_singleton()->register_runtime_scripts();
// ScriptEcs::get_singleton()->register_runtime_scripts();

Choose a reason for hiding this comment

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

is this meant to be disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I remember it caused problems at that point, and we agreed to comment it out for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can check the extra whitespace ones, as long as it passes the static checks

@Beliar83
Copy link
Contributor Author

@RevoluPowered Where exactly are the extra whitespaces? Or is there one missing?

@mkay-dev
Copy link

mkay-dev commented Apr 11, 2023

This makes godex compile with commit 92bee43adba8d2401ef40e2480e53087bcb1eaf1, which is the one that was used to build the stable reelase of Godot 4.0.

Please continue updating Godex while Andrea Catania is busy. There seem to be new issues with compilation due to the following Godot commit godotengine/godot@4e34cf2 .

@Beliar83
Copy link
Contributor Author

@mkay-dev Ah, I guess that was the error on the build I got earlier. I'll try to check that when I am home later today.

@mkay-dev
Copy link

mkay-dev commented Apr 11, 2023

@Beliar83 Thanks in advance. Can I maybe also motivate you to fix some linux issues should they arise. I'm not yet deeply familiar with c++ and godot/godexs source code so I'm only working up to fixing these kind of issues myself.

@Beliar83
Copy link
Contributor Author

Beliar83 commented Apr 11, 2023

@mkay-dev It should generally build on linux (the automatic builds in my fork and this PR do), though I have WSL or a VM at best. If you have any specific problem I can look at it, though.

@mkay-dev
Copy link

@Beliar83 Sounds great. Your fork did compile against Godots 4.0 release build just fine so I'm just being heedful of possible issues. Again thanks for the help.

@mkay-dev
Copy link

mkay-dev commented Apr 12, 2023

Here is the compilation error I am getting:

[ 7%] Generating modules tests header.
[ 26%] Compiling modules/gdscript/gdscript_byte_codegen.cpp ...
[ 26%] Compiling tests/test_main.cpp ...
[ 26%] Compiling modules/gdscript/gdscript_cache.cpp ...
[ 26%] Compiling modules/gdscript/gdscript_compiler.cpp ...
In file included from /home/mkay/projects/godot/from_scratch/modules/godex/tests/test_ecs_pipeline_builder.h:14,
from ./modules/modules_tests.gen.h:12,
from tests/test_main.cpp:113:
/home/mkay/projects/godot/from_scratch/modules/godex/tests/test_utilities.h: In function 'bool register_ecs_script(const StringName&, const String&)':
/home/mkay/projects/godot/from_scratch/modules/godex/tests/test_utilities.h:32: warning: comparison with string literal results in unspecified behavior [-Waddress]
32 | if (extends == "System") {
|
/home/mkay/projects/godot/from_scratch/modules/godex/tests/test_utilities.h:32: error: comparison between distinct pointer types 'GDScriptParser::IdentifierNode*' and 'const char*' lacks a cast [-fpermissive]
[ 26%] Compiling modules/gdscript/gdscript_disassembler.cpp ...
scons: *** [tests/test_main.linuxbsd.editor.x86_64.o] Error 1
scons: building terminated because of errors.
[Time elapsed: 00:00:38.732]

@Beliar83
Copy link
Contributor Author

Beliar83 commented Apr 12, 2023

@mkay-dev I reset my fork and worked on a separate branch for updating the compatibility - as I should have done from the start, so there was nothing changed yet for this PR. I have fixed the problem, but I am doing one more check build after merging from this repo.

@mkay-dev
Copy link

@Beliar83 Excellent! You're a true hero.
Seems to be compiling fine for now. Only had to add missing #include "utility" in /modules/godex/tests/test_ecs_storage_hierarchical.h and /modules/godot/nodes/ecs_world.cpp as I was getting:

error: 'as_const' is not a member of 'std'; did you mean 'is_const'?

@fire
Copy link
Contributor

fire commented Jun 6, 2023

Typically godot engine developers do one merge commit (rebase and then force push to a feature branch). What does @AndreaCatania prefer and how far is this from working?

@fire
Copy link
Contributor

fire commented Jul 9, 2023

Godot 4.1-stable released a few days ago.

@fire fire mentioned this pull request Jul 9, 2023
@fire
Copy link
Contributor

fire commented Jul 9, 2023

https://github.com/Beliar83/godex/blob/main/systems/system.h#L6 is using the wrong type of include.

#include "../ecs_types.h"

@Beliar83
Copy link
Contributor Author

Yeah, I have already updated it in https://github.com/Beliar83/godex/tree/dev_main

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.

None yet

5 participants