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

Move 2D and 3D resources to their own folders #50148

Merged
merged 2 commits into from
Feb 26, 2024

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jul 4, 2021

Similarly to how we have the scene/2d folder and scene/3d folder for 2D and 3D nodes, this PR moves 2D and 3D resources to scene/resources/2d and scene/resources/3d. Also, I moved the skeleton modification resources to another subfolder, scene/resources/2d/skeleton since there were 18 2D skeleton modification resource files. Resources that are shared between 2D and 3D are still in scene/resources.

  • This gives us the ability to easily exclude the resources in these folders when disabling 2D or 3D (but as of this PR, they are always included).

  • In terms of compiled size, as of the current master, the resources folder is the biggest one, so that's another reason that I think it would be nice to divide it up into 2D and 3D subfolders:

godot4-chart

@clayjohn
Copy link
Member

clayjohn commented Aug 3, 2021

SurfaceTool and MeshDataTool should be under /resources rather than /resources/3d. Both can be used in 2D as well as 3D. For example, they can be used with a MeshInstance2D or a MultiMeshInstance2D.

@aaronfranke aaronfranke force-pushed the move-resources branch 2 times, most recently from f235ec8 to c9c7dd3 Compare August 11, 2021 01:19
@aaronfranke aaronfranke force-pushed the move-resources branch 2 times, most recently from c467e7f to 23b29e1 Compare August 18, 2021 00:26
@aaronfranke aaronfranke force-pushed the move-resources branch 2 times, most recently from fd43597 to 38f7ec2 Compare August 28, 2021 03:23
@aaronfranke aaronfranke force-pushed the move-resources branch 2 times, most recently from 8658ce2 to d50b5e6 Compare September 16, 2021 04:13
@aaronfranke aaronfranke force-pushed the move-resources branch 2 times, most recently from 135e4d8 to a60b104 Compare October 3, 2021 20:37
@aaronfranke
Copy link
Member Author

Awhile ago this was brought up in a meeting (should've written this down then, but I forgot), and reduz wondered if instead we should have 2d/resources instead of resources/2d. The challenge here is that there are some resources that are neither 2D or 3D, so we would still need a resources folder. I am proposing this PR as it is because this is the path of least resistance vs the existing codebase, since these files are already in resources, this PR just adds another subfolder, but I am good with either approach, so whatever is preferred we can do that.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

After discussion with maintainers, I agree with the change.

Conceptually, this is fine, a 2D/3D split marginally helps make the code organization better, but also paves the way to more possibility to decrease build size in custom 2D/3D only builds.

My main concern is with breaking compatibility on the include paths for C++ modules which use those Godot resources.

  • For modules which only track a single Godot version (like in-house C++ modules for game-specific logic, etc.), it's fine, the devs can just do the same refactoring when updating to Godot 4.3.
  • For general-purpose open source modules which aim to support multiple Godot branches, it's a bit trickier. They can still wrap things with #if VERSION_HEX >= 0x040300 checks, though that implies including core/version.h in all affected files. Thankfully with GDExtension/godot-cpp, such modules are less common than they were in 3.x, so I think the tradeoff is acceptable (compatibility is not broken for godot-cpp as their the include paths are all flattened to a single folder).

@akien-mga akien-mga merged commit bb6b06c into godotengine:master Feb 26, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks for the work and your patience :)

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.

9 participants