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

Adding Raycast3D custom debug shape thickness and color #46529

Merged

Conversation

jmb462
Copy link
Contributor

@jmb462 jmb462 commented Feb 28, 2021

Bugsquad edit: master version of #49726.

What is the problem solved by this PR and use cases ?

During the development of a game with my teammate @fabriceci, we had a hard time with the "debug shape" of Raycast3D.
A Raycast is a bit special, because contrary to CollisionShape, they don’t “cover a mesh” which you can easily refer to.

This leads to make difficult the debugging of certain Raycast3D :

  • the Raycast3D is represented by a white line, it’s often hard to see it (collapsed by the grid, aligned with the axis...)
  • if we have multiple Raycasts on a Node, it’s hard to make the distinction between them. For example, a ball rotate/cube and you want to keep track of all directions with Raycast3D, you can’t.
  • If the background is white or bright, the Raycast become nearly invisible.
  • You can't see the direction of the ray
    before

How does this Pull Request fix these problems ?

This PR is done in the simplest way possible by: adding 2 optional parameters to be able to set a per Raycast3D custom color and / or thickness.

By default, the behaviour is the same as now, the thickness is set by default to 1 which draw a line and default color is the same as current default color.

The goal is to allow highlighting some chosen Raycast3Ds in order to make debugging easier.
When the thickness is more than 1, a parallelepiped is drawn, instead of the line, with a bit larger base than the target point to show the direction.
after

raycast1280.mp4

@Calinou
Copy link
Member

Calinou commented Feb 28, 2021

Related to #40123.

@jmb462 jmb462 requested review from Calinou and removed request for a team March 1, 2021 11:17
@pouleyKetchoupp pouleyKetchoupp requested a review from a team March 1, 2021 14:57
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

The idea is great, I think it will improve readability a lot.

I've left some comments on the implementation, let me know if anything is not clear enough.

In general, I wonder if we could just always set things with a thick shape, since the single line is very difficult to see. It could be simpler in this case, we could even get rid of setting the thickness and just always set things the way it is with a thickness of 2 right now, I think it would look great.

@fabriceci
Copy link
Contributor

It could be simpler in this case, we could even get rid of setting the thickness and just always set things the way it is with a thickness of 2 right now, I think it would look great.

@pouleyKetchoupp There is one edge case where a line is better than the parallelepiped: when the camera is far away:

image

We assumed it was normal as lines thickness are not shrunk by the camera distance, contrary to a mesh (are we misunderstanding something !?).

If we want to make this shape the default, we need to overcome this. We thought to solve this by drawing a line AND a mesh. (OR using camera distance (if possible) to adapt mesh thickness with the distance)

What is your opinion on that? Is there a better solution?

@pouleyKetchoupp
Copy link
Contributor

@fabriceci I didn't think about camera distance, and it does make sense that a line would render better in that situation.

Doing both a line and a mesh might help. I wonder if it would make the code over-complicated though. Feel free to try if you want, and if it's easy enough and looks ok with transparency it would be great.

Otherwise you can keep the thickness parameter like you did, it would be fine. Let's just make at least 2 the default value since it would look better in common cases.

@jmb462 jmb462 requested review from a team as code owners March 1, 2021 23:08
@pouleyKetchoupp pouleyKetchoupp removed request for a team March 1, 2021 23:16
@jmb462 jmb462 force-pushed the improvement-raycast3d-debug-shape branch from 4b07d94 to 7805b09 Compare March 2, 2021 00:06
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

It looks good with what you did with rendering both the line and the parallelepiped, so it's almost there!

Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

I've made comments on a few more things, mostly details and things I've missed before.

Once the last changes are done, please squash your commits using this workflow:
https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html?#the-interactive-rebase

And then it should be good to go :)

@jmb462 jmb462 force-pushed the improvement-raycast3d-debug-shape branch from a37f33c to 068300c Compare March 4, 2021 10:21
@akien-mga akien-mga dismissed Calinou’s stale review March 4, 2021 19:38

Changes done.

@akien-mga akien-mga merged commit e556ec0 into godotengine:master Mar 4, 2021
@akien-mga
Copy link
Member

Thanks!

@jmb462
Copy link
Contributor Author

jmb462 commented Mar 4, 2021

A big thanks to @Calinou and @pouleyKetchoupp for their precious help !
We (@fabriceci and me) have learned a lot working on this PR.

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.

5 participants