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.

Sign up for GitHub

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

Jump to bottom

Rename Navigation{Mesh,Polygon}Instance and PlaneShape for clarity #36388

Merged
merged 3 commits into from
Feb 28, 2020
Merged

Rename Navigation{Mesh,Polygon}Instance and PlaneShape for clarity #36388

merged 3 commits into from
Feb 28, 2020

Conversation

AndreaCatania
Copy link
Contributor

@AndreaCatania AndreaCatania commented Feb 20, 2020 •
edited

Renamed:

  • NavigationMeshInstance -> NavigationRegion
  • PlaneShape -> WorldMarginShape
  • NavigationPolygonInstance -> Navigation2DRegion

@AndreaCatania AndreaCatania added this to the 4.0 milestone Feb 20, 2020
@AndreaCatania AndreaCatania requested a review from a team as a code owner February 20, 2020 17:08
@Calinou
Copy link
Member

Calinou commented Feb 20, 2020 •
edited

PlaneShape -> WorldMarginShape

WorldMarginShape is less confusing than PlaneShape, but what about renaming it to InfinitePlaneShape so that existing users can find the new class more easily? Just a thought.

@aaronfranke
Copy link
Member

Why not include the 3D suffix? Would be strange to rename them here and then again in #30736

@AndreaCatania
Copy link
Contributor Author

AndreaCatania commented Feb 20, 2020 •
edited

@Calinou the InfinitePlane doesn't solve the main problem because user will continue to use it as ground. For me InfinitePlaneShape is nice but I feel WorldMarginShape to be much more clear and capable to express the why that shape exist.

@aaronfranke yes, but I prefer that 3D is added all together with the PR proposal so that change is uniform.

@AndreaCatania AndreaCatania force-pushed the some_renames branch 2 times, most recently from ddbbcb7 to 88a14b5 Compare February 21, 2020 11:30
@AndreaCatania
Copy link
Contributor Author

Also renamed NavigationPolygonInstance to Navigation2DRegion

@akien-mga
Copy link
Member

Also renamed NavigationPolygonInstance to Navigation2DRegion

Shouldn't it be NavigationRegion2D? Most 2D nodes have 2D as suffix, and we'll eventually add 3D suffixes to 3D nodes, so we'd also have NavigationRegion3D.

@AndreaCatania
Copy link
Contributor Author

Yes, correct - I followed the server name convention, I'll rename it.

@AndreaCatania
Copy link
Contributor Author

Name fixed, @akien-mga .

@akien-mga
Copy link
Member

Can you add the old names as compatibility aliases in scene/register_scene_types.cpp? Grep for ClassDB::add_compatibility_class.

akien-mga
akien-mga reviewed Feb 28, 2020
View reviewed changes
scene/resources/world_margin_shape.h Outdated
@@ -28,14 +28,14 @@
/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
/*************************************************************************/

#ifndef PLANE_SHAPE_H
#define PLANE_SHAPE_H
#ifndef world_margin_shape_H
Copy link
Member

Choose a reason for hiding this comment

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

Still lowercase here.

@akien-mga akien-mga changed the title Some renames Rename Navigation{Mesh,Polygon}Instance and PlaneShape for clarity Feb 28, 2020
@AndreaCatania
Copy link
Contributor Author

All right! fixed

@akien-mga akien-mga merged commit 4f64f34 into godotengine:master Feb 28, 2020
@akien-mga
Copy link
Member

Thanks!

@AndreaCatania AndreaCatania deleted the some_renames branch February 28, 2020 08:20
@lupoDharkael lupoDharkael mentioned this pull request Mar 8, 2020
Merged
@Calinou Calinou mentioned this pull request Aug 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
breaks compat enhancement topic:core topic:navigation
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@AndreaCatania @Calinou @aaronfranke @akien-mga