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

Extract and reorganize texture resource classes #68460

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

Geometror
Copy link
Member

@Geometror Geometror commented Nov 9, 2022

Split up texture.h/cpp into:

  • texture.h/cpp (now only contains Texture, Texture2D, TextureLayered, Texture3D)
  • animated_texture.h/cpp
  • atlas_texture.h/cpp
  • camera_texture.h/cpp
  • compressed_texture.h/cpp
  • curve_texture.h/cpp
  • image_texture.h/cpp
  • gradient_texture.h/cpp
  • mesh_texture.h/cpp
  • portable_compressed_texture.h/cpp
  • placeholder_textures.h (contains all PlaceholderXXX classes since they are very small)
  • proxy_texture.h/cpp [got removed]

1D/2D/3D variants are grouped together in one file because that made the most sense to me, but if we wan't to be really strict (one class per file), I could split them up as well.
Part of the ongoing effort to have only one class per file (or at least have smaller source files) with the goal of improving the structure of the codebase in terms of readability and compilation speed.
texture.h/texture.cpp were especially problematic since the header was around 1100 LOC long and the source file over 3500 LOC.
Also introducing forward declarations of some classes to further improve compilation speed.

@Geometror Geometror added this to the 4.0 milestone Nov 9, 2022
@Geometror Geometror requested review from a team as code owners November 9, 2022 19:33
@Geometror Geometror force-pushed the split_texture_src branch 2 times, most recently from e51fb0e to dbe9174 Compare November 9, 2022 19:56
@fire
Copy link
Member

fire commented Nov 9, 2022

I am ok with this push for one class per cpp. Other people thoughts?

@Geometror Geometror force-pushed the split_texture_src branch 2 times, most recently from 36da0ab to 1472827 Compare November 10, 2022 02:13
scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
@YuriSizov YuriSizov requested review from a team and removed request for a team July 10, 2023 14:18
@YuriSizov
Copy link
Contributor

@Geometror I think we are ready to merge it in the next couple of weeks. So if you could do a rebase, it would be great!

@Geometror Geometror force-pushed the split_texture_src branch 4 times, most recently from 9aa6f1a to 9d07e9e Compare July 11, 2023 22:30
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

A couple of style notes, but otherwise this is good to go!

@YuriSizov YuriSizov merged commit a758388 into godotengine:master Jul 14, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks! Love me some healthy and maintainable codebase <3

@@ -0,0 +1,203 @@
/**************************************************************************/
/* image_texture.h */
Copy link
Member

Choose a reason for hiding this comment

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

Why not put these in a textures subfolder?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could do that.

@Geometror Geometror deleted the split_texture_src branch July 14, 2023 21:38
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.

4 participants