Cleanup of OpenXR module SCons config #86537
Conversation
|
I could use some pointers here by scons aficionados, I'm sure some of the SCsub files can be done better (cc @akien-mga ). The main goal here was to not have to list all the extensions as we're slowly adding more. |
| #include "../action_map/openxr_interaction_profile_metadata.h" | ||
| #include "modules/openxr/action_map/openxr_interaction_profile_metadata.h" |
There was a problem hiding this comment.
This may be a personal preference thing, but because these includes are all within the same module, I personally like the relative includes better than the absolute. Doing a quick grep through the rest of the code, it seems like other modules also prefer including other things within the same module using relative paths. Not sure this actually matters, though :-)
There was a problem hiding this comment.
The code style does demand absolute paths, and I believe it additionally caused issues with SCU
There was a problem hiding this comment.
Hm, ok. I guess it's one of those cases where existing code hasn't been updated to match the code style yet?
Grepping for include.*\.\. shows lots of includes for headers inside of the same driver or module. Whereas grepping for include.*modules/ only shows includes to modules from outside a given module. I don't see any current examples of a module including headers within the same module using an absolute path.
There was a problem hiding this comment.
It does seem unclear, the code style should probably be updated here
There was a problem hiding this comment.
I'm really on the fence, I agree with the absolute paths but we have started to use local paths especially with modules. It's also normal for the include file that belongs with a cpp file to not have the path.
But there doesn't seem to be much rule to it.
Here I've changed it to absolute paths because we now have extensions in the platform folder (those that are not always compiled in) and in the extensions folder (those that are always compiled), and where they include the same files I personally prefer it if the code looks similar.
When it comes to text, I'm very visual, I see patterns in what is written, so if things are different when they should be the same, it stands out like a sore thumb.
There was a problem hiding this comment.
The higher authority is me :P
But I haven't had time to review now, I'll assess soon. I can just point out that
we now have extensions in the platform folder (those that are not always compiled in) and in the extensions folder (those that are always compiled)
May be going backward with regard to the kind of encapulsation we've been striving to get with platforms and modules. It's a bit of a chicken and egg problem since both are meant to be pluggable/modular, but so far the status quo seemed to be that modules should be the most modular part that can be taken in/out at will, so it should contain its own conditionally compiled platform code.
Modules config.py include information about the env and platform so they can be configured conditionally, while platform detect.py in theory shouldn't know much about the internals of a module. It's not always been fully respected, e.g. we had some gcc optimization flags for theora set in the Linux detect.py, but I would consider this a bug nowadays.
Anyway, TIWAGOS, I haven't reviewed any of the changes in this PR yet.
But generally speaking, we strive to use relative paths within modules and platforms for their respective "own" files, with the idea that the module/platform can be renamed/removed and everything should still all compile fine.
There was a problem hiding this comment.
The code style does demand absolute paths, and I believe it additionally caused issues with SCU
The code style hasn't been updated yet to take these refactorings into account:
- Style: Harmonize header includes in platform ports #75932
- Style: Harmonize header includes in modules #75944
These should be taken as the reference of how includes should be handled in modules and platforms. It's slightly convoluted, because the various aspects we're trying to convey are more complex than it looks :)
There was a problem hiding this comment.
I've changed it to relative paths, @akien-mga if you have time to take another look and let me know if you think this is good :)
There was a problem hiding this comment.
Looking at the current PR, I'm still seeing lots of paths starting with "modules/openxr/..." (what we've been calling "absolute", but I guess is technically still relative, but relative to the top of the Godot source code) rather than relative to source file (which we've been calling "relative").
There was a problem hiding this comment.
ah crap, I only checked the header files..
ok got them all this time :)
BastiaanOlij
force-pushed
the
openxr_cleanup_scons
branch
2 times, most recently
from
9b04a0f
to
f1dc618
Compare
BastiaanOlij
force-pushed
the
openxr_cleanup_scons
branch
from
f1dc618
to
f3eaf65
Compare
BastiaanOlij
force-pushed
the
openxr_cleanup_scons
branch
from
f3eaf65
to
82a36cf
Compare
akien-mga
changed the title
|
Thanks! |
This PR makes a few changes to the scons setup in the OpenXR modules:
extensions/platform*.cppfiles in theextensionsfolder are now compiled and linked, they no longer need to be individually addedaction_map,extensionsandscenenow have their ownSCsubfile