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

Generate compatibility layer #1415

Open
wants to merge 1 commit into
base: 4.3
Choose a base branch
from
Open

Conversation

Ughuuu
Copy link

@Ughuuu Ughuuu commented Mar 15, 2024

Generate compatibility layer between godot and godot_cpp at GDExtension level.

@AThousandShips AThousandShips changed the title generate compat layer Generate compatibility layer Mar 15, 2024
@AThousandShips AThousandShips added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels Mar 15, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Mar 15, 2024
@Ughuuu
Copy link
Author

Ughuuu commented Mar 15, 2024

Actually no need to change the ci since we dont export the headers just the lib atm.

@Ughuuu Ughuuu force-pushed the compat-layer branch 4 times, most recently from c531aa8 to 841edd2 Compare April 6, 2024 17:43
@Ughuuu Ughuuu changed the base branch from master to 4.3 October 20, 2024 09:45
@Ughuuu Ughuuu force-pushed the compat-layer branch 2 times, most recently from 7097bd2 to 6dc467a Compare October 20, 2024 09:47
@Ughuuu
Copy link
Author

Ughuuu commented Oct 20, 2024

Should the output_header_mapping_godot.json file be in the repo at root(or should there be other ways of generating this file every time)? If so it would need to be manually generated each time based on similar/matching godot version (eg. godot-cpp 4.3, godot 4.3)
As for generating it every time, a param could be given to the scons run command (probl as env var) as to where the godot repo is and run the command there and generate the bindings then?
Eg. if you set GODOT_REPO_PATH then the godot_compat bindings are generated, if not no. I'll give it a try and see if it works well will do it like that.

Edit:

Made it work without needing the file commited, just needs to have the godot_repo set, eg.:

scons godot_repo="../godot"

@dsnopek
Copy link
Collaborator

dsnopek commented Oct 21, 2024

Given that this is mapping godot-cpp-style includes to Godot includes, could this be implemented by changing our normal .hpp headers to instead include the Godot header when built as a module?

So, for example, could it have godot_cpp/classes/xr_server.hpp like:

#ifdef GODOT_MODULE
#include "servers/xr_server.h"
#else

// All the rest of the code that's normally in xr_server.hpp...

#endif

Then a GDExtension doesn't actually need to change its includes to build as a module.

@Ughuuu
Copy link
Author

Ughuuu commented Oct 21, 2024

Very good idea, must say, has some downsides, but overall a lot of upsides.
This might work for the generated stuff, but won't work for the non generated stuff (eg files in include folder, such as variant/variant.hpp, etc. These need to be manually changed). If the manual ones are also updated, then it will work and won't need any mapping file.
However, this will have the downside of tying godot-cpp to godot version. Which for eg. if you work with godot-cpp 4.3 and godot 4.4, and godot changed location of some files, then it won't work.

Another thing I forgot, godot uses no namespacing, while godot-cpp uses using namespace godot;. Right now I added in the godot_compat incldues the using namespace godot; to polute the global namespace to have symetry.

@dsnopek
Copy link
Collaborator

dsnopek commented Oct 21, 2024

If the manual ones are also updated, then it will work and won't need any mapping file.

We could have the hand-written files include a generated file that would have the Godot includes (or nothing, if the user didn't pass godot_repo="...").

However, this will have the downside of tying godot-cpp to godot version. Which for eg. if you work with godot-cpp 4.3 and godot 4.4, and godot changed location of some files, then it won't work.

Well, if this code is only added if the developer specified godot_repo="...", then godot-cpp isn't bound to any Godot version unless requested.

@Ughuuu
Copy link
Author

Ughuuu commented Oct 21, 2024

Sounds good to me, so then I start to implement that change where everything just works magically inside the godot_cpp includes?

  1. the generated files will have (if there is a godot_repo is passed) both addon and module.
  2. the manual files will get also overridden (at generation time? if godot_repo is set and basically add the ifdef for addon and module as well)

Also, this will only happen if godot_repo is sent, so no extra file anywhere. Also, I update both SConstruct and CMake with this extra godot_repo param.

EDIT:

And about the using namespace godot; that is only in godot-cpp (As addon) how will that be solved? Right now I do (eg.):

#ifdef GODOT_MODULE_COMPAT
#include <core/math/a_star_grid_2d.h>
#else
#include <godot_cpp/classes/a_star_grid2d.hpp>
using namespace godot;
#endif

@dsnopek
Copy link
Collaborator

dsnopek commented Oct 21, 2024

2. the manual files will get also overridden (at generation time? if godot_repo is set and basically add the ifdef for addon and module as well)

Just to be clear, I don't think the manual files need to get "overridden", they just need to have code like this added:

#ifdef GODOT_MODULE
// The next included file is generated: if we had `godot_repo="..."` it'll contain the Godot includes, if not, it'll just be an empty file.
#include <godot_cpp/compat/HEADER.hpp>
#else

// ... the code that's there now ...

#endif

So, they'd still be hand-written, but they'd include a generated file. We definitely shouldn't have the generation touch the content of the hand-written files.

@Ughuuu
Copy link
Author

Ughuuu commented Oct 22, 2024

Making it draft while I implement new changes.

@Ughuuu Ughuuu marked this pull request as draft October 22, 2024 15:55
@Ughuuu Ughuuu marked this pull request as ready for review October 26, 2024 21:48
Copy link
Collaborator

@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.

Thanks!

I think the overall idea of the new direction this PR is taking seems good. However, there's some things in the implementation that I think could be improved (more details below), and there still appears to be a bunch of stuff left over from the previous direction.

include/godot_cpp/variant/callable_method_pointer.hpp Outdated Show resolved Hide resolved
binding_generator.py Outdated Show resolved Hide resolved
Comment on lines 411 to 412
if godot_repo != "":
generate_compat_includes(godot_repo, output_dir, target_dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this is modifying the generated files afterwards? Why not just make this part of generating the files in the first place?

Copy link
Author

Choose a reason for hiding this comment

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

There are a lot of functions that create the file. Also this is how it was previously as I created the file in another folder. I can update if you want, but that will polute the smaller functions (that generate file in the first place)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I think we should update the functions that generate the files in the first place

Copy link
Author

Choose a reason for hiding this comment

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

Ok, well, got it working now all around. If you want this change, I can do that instead, but first let me bring some counter arguments I guess. If in future we don't need this tight integration anymore, it's easier to remove. All the code for compat is in one function. If there are bugs/issues with it, its one function to debug, and not multiple functions.
Downsides to having it separate is that people might not test it as often and it might break, though since in other functions it will be under if's anyway, won't be much tighter integrated anyway.

So yeah, if you think it's better to integrate them in functions, will update again the code. Just double checking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really do think we should integrate this into the functions generating the files in the first place. But I guess that's just my opinion? If you can get other GDExtension team members on-board, I'd be fine to go with the consensus, but I personally think it's silly to modify files after the fact, when it's the same script that's generating them in the first place.

Copy link
Author

@Ughuuu Ughuuu Oct 29, 2024

Choose a reason for hiding this comment

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

Who are other gdextension members? Should i request a review on this pr at this stage?

Copy link
Collaborator

Choose a reason for hiding this comment

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

binding_generator.py Outdated Show resolved Hide resolved
@Ughuuu Ughuuu force-pushed the compat-layer branch 2 times, most recently from 01db9c6 to f660942 Compare October 29, 2024 21:01
generate compat

generate compat

Update ci.yml

Update binding_generator.py

generate compat

generate compat

lint python files

Update compat_generator.py

update docs

Update binding_generator.py

Update module_converter.py

also collect defines

Add module converter file that converts module based projects to godot_compat

Update ci.yml

update docs

Update compat_generator.py

lint python files

generate compat

generate compat

generate compat

generate compat

Update ci.yml

fix path issue when caling from outside

Update binding_generator.py

update to also take missing classes/structs

Update binding_generator.py

Generate godot compat for dual build

generate compat

generate compat

Update ci.yml

Update binding_generator.py

generate compat

generate compat

lint python files

Update compat_generator.py

update docs

Update binding_generator.py

Update module_converter.py

also collect defines

Add module converter file that converts module based projects to godot_compat

Update ci.yml

update docs

Update compat_generator.py

lint python files

generate compat

generate compat

generate compat

generate compat

Update ci.yml

fix path issue when caling from outside

Add support for build profiles.

Allow enabling or disabling specific classes (which will not be built).

Allow forwarding from `ClassDB` to `ClassDBSingleton` to support enumerations

update to also take missing classes/structs

Update binding_generator.py

update

update naming of files

add godot mappings.

update and run output_header_mapping.json

Update README.md

make godot_compat work without a file generated

fix the test

Update binding_generator.py

Update binding_generator.py

Update binding_generator.py

use files from include too

Update README.md

lint

lint

lint

Update CMakeLists.txt

update to use all. fix linting a bit

update wip

fix posix path

Update CMakeLists.txt

Update binding_generator.py

add using namespace godot; everywhere to includes

fix includes

Try fixes.

generate new include files 123

Update binding_generator.py

Update binding_generator.py

Update binding_generator.py

Update binding_generator.py

update

fix GODOT_MODULE_COMPAT

fix manual includes to match.

Update godot.hpp

Update color_names.inc.hpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an easier way to support GDExtensions for both addons and modules, specifically for includes
3 participants