godot
godot copied to clipboard
godotengine
Don't include `core/io/image.h` in `core/os/os.h`
core/os/os.h doesn't use core/io/image.h
It just brings to the code base transitive dependencies. Lots of dependencies because core/os/os.h is transitively included in almost every file of godot
How numbers were obtained. First of all thanks to this message in chat I knew about --tree=linedraw. So after compiling I run scons --tree=linedraw > /tmpt/godot.linedraw after that I needed to get actual number of includes of tested files. By running grep "<file>" /tmp/godot.linedraw | wc -l you get a number of files which directly or transitively depend on <file>
core/os/os.h |
core/io/image.h |
|
|---|---|---|
| Master | 3040 | 3073 |
| PR | 3040 | 2650 |
Summary. With this PR compiler will need to process core/io/image.h 423 times less. Numbers were obtained on linux and will differ a little on different platforms because they doesn't share files inside platform folder.
It definitely should improve compilation speed but how much.... Maybe several microseconds, maybe several minutes. I can't check. One full build build from clean state takes 1.5~ hours on my machine so if you have a more powerful machine, could you please test if there is difference in compile times?
This PR also contains one minor change. core/core_bind.h didn't use core/io/image.h in any way as like all who includes it so this change doesn't depend core/os/os.h file
I honestly doesn't know anything about Ref<Image> and didn't saw compiler errors which involve Ref at all and don't know what you are talking about ¯\_(ツ)_/¯
This include is likely an old one from 3.x, a bunch of Ref<Image> function were in OS, but now are in DisplatyServer.
The issue is explained here https://github.com/godotengine/godot/pull/80330#issuecomment-1666840873
Honestly I can't say that I really understand Ref problem. Is the problem applicable only for forward declared things?
I removed forward declaration of Image inside display_server.h because it already included os.h which included image.h so IIUC there couldn't be problem with that forward declaration because the full definition also was pulled through os.h. Now there is no forward declaration at all so I want to believe there shouldn't be problem you describe.
Are you sure there are no places that used to include this that have Ref<Image> and don't include the header? If yes then there's no problem, otherwise it is a risk and should have an include in the header as a replacement
Are you sure there are no places that used to include this that have
Ref<Image>and don't include the header? If yes then there's no problem, otherwise it is a risk and should have an include in the header as a replacement
It looks like there are some places where the potential problem exist. I am almost found an easy way to find them with grep and etc so I think I will solve them tomorrow. Thank you for explanations :smiley:
Are you sure there are no places that used to include this that have
Ref<Image>and don't include the header? If yes then there's no problem, otherwise it is a risk and should have an include in the header as a replacement
Now I am sure. Though I didn't add it to the files which include core/io/image.h or core/io/image_loader.h directly inside .cpp or in corresponding .h file with the same name
