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

Rename RayShape to SeparationRayShape #711

Closed
Xrayez opened this issue Apr 15, 2020 · 6 comments · Fixed by godotengine/godot#51896
Closed

Rename RayShape to SeparationRayShape #711

Xrayez opened this issue Apr 15, 2020 · 6 comments · Fixed by godotengine/godot#51896
Milestone

Comments

@Xrayez
Copy link
Contributor

Xrayez commented Apr 15, 2020

Describe the project you are working on:
Godot Engine

Describe the problem or limitation you are having in your project:
There are certain confusion over the real intention and usage of the classes.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
Renaming the classes to reflect the intended usage shall resolve the confusion. In this case, explicit is better than implicit.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
Renaming is already suggested by official article as in:
https://godotengine.org/article/godot-31-will-get-many-improvements-kinematicbody

RayCast shapes separate the body in the direction they are pointed to, and will always return a collision normal along their ray, instead of the one provided by the floor (because, again, their goal is to separate). Yes, the name is confusing, maybe they should be renamed to SeparationRayShape in Godot 4.0 :)

Alternatively, it may be worth implementing #333 and just rename this to SeparationShape which would accept any existing shape Shape (or just allow any shape to act as separation shape for that matter). Marginally related proposal for ShapeCast node: #710.

If this enhancement will not be used often, can it be worked around with a few lines of script?:
Possible to create a script wrapper over the existing class.

Is there a reason why this should be core and not an add-on in the asset library?:
People should not get confused between ray casting and ray shapes used for physics separation and should be advertised by the engine devs instead.

@Jummit
Copy link

Jummit commented Apr 15, 2020

godotengine/godot#16863

@Calinou
Copy link
Member

Calinou commented Apr 15, 2020

Tracking renames is probably better done in the issue @Jummit linked. You generally don't need to write a formal proposal for this 🙂

@Xrayez
Copy link
Contributor Author

Xrayez commented Apr 15, 2020

@Calinou do you think same can be said for #652?

It's just that it's likely to attract controversial discussions and whatnot, which worth an entire proposal. 🙂

@Xrayez
Copy link
Contributor Author

Xrayez commented May 31, 2020

This is probably better to be tracked by godotengine/godot#16863 as suggested. I've done some refactoring to make the transition more consistent: godotengine/godot#39209. I feel like the renaming itself asks for more underlying renamings, and I don't feel like doing this myself (honestly). 🙂

@Xrayez Xrayez closed this as completed May 31, 2020
@Zylann
Copy link

Zylann commented May 31, 2020

I'm actually confused by this new name. Why SeparationRayShape? Where does the "separation" comes from? In the context of a game engine I always thought "ray" was enough to describe a point with a direction, max distance and one possible hit. If some newcomers get to use such a shape and not understand that, it sounds like a documentation issue. IMO, what may be confusing more than the name, is the fact considering "ray" to be a "shape", it's a bit of a stretch. If the confusion comes from the single-hit, then it could internally be a line-cast with results ordered by distance, if more than one result is wanted, dunno.

@Xrayez Xrayez reopened this Aug 19, 2021
@Xrayez
Copy link
Contributor Author

Xrayez commented Aug 19, 2021

There's a PR godotengine/godot#51896, so I assume this change is desired, so reopened.

@akien-mga akien-mga added this to the 4.0 milestone Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants