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

Use forward-declarations in EditorPlugin where possible #60684

Merged
merged 1 commit into from
Nov 13, 2022

Conversation

trollodel
Copy link
Contributor

@trollodel trollodel commented May 1, 2022

Extracted from #58506.
Follow up of #57306.
Follow up of #58136.

Reduce the number of EditorPlugin includes as much as possible (3 for now) to reduce the risks of recompiling all the editor plugins (so half the editor) by doing an unrelated changed. It probably speeds up a bit the compile time.

Also add some forward declarations in touched headers.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 18, 2022

I see some new includes in headers. I think you could get rid of them once #65062 is merged, because EditorUndoRedoManager has some include hell right now.

@trollodel
Copy link
Contributor Author

Do you mean UndoRedo includes? I can remove them in the next rebase.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 18, 2022

@trollodel
Copy link
Contributor Author

trollodel commented Sep 18, 2022

E.g. this
#60684 (files)
or this
#60684 (files)

The files you linked aren't EditorPlugins, so I need to check if the changes are necessary at all.

and here you even removed forward-declaration for some reason:
#60684 (files)

EditorUndoRedoManager needs to be included (it's done above), because IIRC it fails to compile on plugin registration if it's not included.

That said, there may be some unused includes now due to changes in modified files.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 18, 2022

EditorUndoRedoManager needs to be included (it's done above), because IIRC it fails to compile on plugin registration if it's not included.

That's what the PR I mentioned solves.

@trollodel
Copy link
Contributor Author

EditorUndoRedoManager needs to be included (it's done above), because IIRC it fails to compile on plugin registration if it's not included.

That's what the PR I mentioned solves.

Oh, I see. Yes, I can remove it once that PR is merged.

@trollodel trollodel force-pushed the lightweight_editor_plugin branch from f73ab09 to 9a9e1a6 Compare November 2, 2022 14:36
@trollodel trollodel force-pushed the lightweight_editor_plugin branch 2 times, most recently from 8d96567 to faf3eaa Compare November 9, 2022 16:55
@trollodel
Copy link
Contributor Author

EditorUndoRedoManager needs to be included (it's done above), because IIRC it fails to compile on plugin registration if it's not included.

That's what the PR I mentioned solves.

Oh, I see. Yes, I can remove it once that PR is merged.

@KoBeWi I fixed the comment about EditorUndoRedoManager. I also checked changes in non editor plugin files, and they are needed because they include indirectly editor_plugin.h.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 10, 2022

I still don't get why you added new includes. If something is required indirectly, it can still be cleaned up, until everything compiles.

I left one comment, but this probably applies to all new includes. I checked myself that it can be removed.

@trollodel
Copy link
Contributor Author

trollodel commented Nov 11, 2022

Let me clarify, editor_plugin.h used to include indirectly lots of stuff. Here I removed most of them and replace its dependencies with forward declarations (as for PR title). Then I fixed every unsatisfied dependencies until it compiles. I used includes (for the most part) to be consistent with existing code. But as you said, every new include can be replaced with forward declarations and I can do it if wanted.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 11, 2022

But as you said, every new include can be replaced with forward declarations and I can do it if wanted.

Well, that's the purpose of this PR, no? We'll eventually want to do it everywhere, so it's ok if it goes a bit out of scope.

@trollodel trollodel force-pushed the lightweight_editor_plugin branch from faf3eaa to 8fe5061 Compare November 11, 2022 19:17
@trollodel trollodel force-pushed the lightweight_editor_plugin branch from 8fe5061 to ba9e619 Compare November 11, 2022 19:26
@trollodel
Copy link
Contributor Author

Well, that's the purpose of this PR, no? We'll eventually want to do it everywhere, so it's ok if it goes a bit out of scope.

Not really. Anyway, I changed all the includes (where possible) with forward declarations and also reorder them in some places.

@akien-mga akien-mga merged commit d75018b into godotengine:master Nov 13, 2022
@akien-mga
Copy link
Member

Thanks!

@trollodel trollodel deleted the lightweight_editor_plugin branch November 13, 2022 15:03
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.

3 participants