godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.1k stars 69 forks source link

Resolving method naming inconsistencies in Geometry singleton for 2D and 3D #640

Closed Xrayez closed 4 years ago

Xrayez commented 4 years ago

TL;DR: lets just create Geometry2D and Geometry3D singletons.


Describe the project you are working on: Destructible terrain implementation in 2D.

Describe the problem or limitation you are having in your project: With some 2D geometry features added in godotengine/godot#28987, godotengine/godot#29127 it starts to become more difficult to tell which methods are intended to be used for either 2D or 3D, or names become inconsistent due to collisions. For instance clip_polygon vs clip_polygons_2d.

Upon stumbling clip_polygon I'd expect it to operate in 2D, but then realize it's used in 3D. Polygon term doesn't seem to be used much in 3D field either, but rather as a polygonal mesh or faces (excuse my lack of terminology on this, please correct me if I'm wrong).

A lot of geometry functions are analogously available in both 2D in 3D:

geometry_methods_discrimination

For less trivial methods, see 2D/3D/nD Delaunay triangulation (Godot doesn't provide 3D Delaunay triangulation as of now though).

Describe the feature / enhancement and how it helps to overcome the problem or limitation:

I see two possible solutions which can both resolve naming collisions:

1. Rename 2D methods by removing _2d suffix

This creates a collision with 3D methods of course. For that I personally propose to create two dedicated geometry singletons: Geometry2D and Geometry3D and move methods there:

2. Rename 3D methods by appending _3d suffix

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams: I guess either solution can work depending on Godot priorities, but I personally prefer if we decide to go for the (1) splitting solution.

It just takes some copy-pasting of existing code to geometry_2d.cpp and geometry_3d.cpp, or both classes can reside in a single file instead (which may be less work).

If this enhancement will not be used often, can it be worked around with a few lines of script?: One could create a class_name global class wrappers over the Geometry singleton.

Is there a reason why this should be core and not an add-on in the asset library?: This is mostly inspired by godotengine/godot#30736, so I guess it makes sense to refactor this similarly in core.

This would also be a nice opportunity to move implementation details outside of the header file geometry.h to geometry.cpp/geometry_2d.cpp/geometry_3d.cpp which can prevent rebuilding most of the engine on the slightest change.

List of dependencies:

These issues/PRs need to be reviewed first before any work can be done on this.

Xrayez commented 4 years ago

Could someone transfer this to GIP please? Perhaps @reduz can do something about this having the work done on renaming 3D nodes as in godotengine/godot#37340 already, thanks. Otherwise feel free to close this.

akien-mga commented 4 years ago

Transferred to proposals repo.

I think it's a good idea to make two singletons.

fire commented 4 years ago

@Xrayez Since this is transferred, can you adjust the proposal to fit the template? This is just for consistency.

akien-mga commented 4 years ago

@reduz also approved this on IRC, as well as the implementation PR godotengine/godot#39051, so this can be considered approved by core devs.