Adds forward declaration instead of the include. Fixes a bug, when a ray inside the CBoundingSphere returns false for RayIntersect. Adds const qualifier for the constant getter.
Details
Details
- Reviewers
wraitii - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22143: Refactor CBoundingSphere and add tests
- Apply the patch and compile the game
- Run tests and make sure that all works
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 6249 Build 10374: Vulcan Build Jenkins
Event Timeline
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/648/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/649/display/redirect
Comment Actions
Does it really make sense to talk about 'inside' for infinite rays?
I guess the bug you are fixing is when the ray's origin is inside the sphere (I kinda suck at geometry) ? So maybe clarify 'starting inside'?
Tests pass and code seems sensible otherwise.
| source/maths/BoundingSphere.h | ||
|---|---|---|
| 23 | <3 | |
Comment Actions
Yes. Because it won't be handled as intersection currently.
So maybe clarify 'starting inside'?
Do you suggest to assume that we don't have rays inside spheres, and if have then it's not the intersection, because we don't see them?
| source/maths/BoundingSphere.cpp | ||
|---|---|---|
| 48 | Oh okay. Why do you have to compare the v.LengthSquared(), isn't there just length ? in this case you wouldn't need the multiplication at all | |
| source/maths/BoundingSphere.cpp | ||
|---|---|---|
| 48 | The length of a vector is length = sqrt(x * x + y * y + z * z). So if you want to compare the length and a number, you write if (length < number). If we remove the square root (because square is usually slower than the multiplication), we use the squared length: if (x * x + y * y + z * z < number * number). | |
| source/maths/BoundingSphere.cpp | ||
|---|---|---|
| 48 | So so squared means square rooted here not Math Pow 2 ? | |
| source/maths/BoundingSphere.cpp | ||
|---|---|---|
| 48 | No, it means that the square root was removed :) Because square root of a number in square is the number. | |
| source/maths/BoundingSphere.cpp | ||
|---|---|---|
| 48 | Oh yeah right :) | |
Comment Actions
So maybe clarify 'starting inside'?
Do you suggest to assume that we don't have rays inside spheres, and if have then it's not the intersection, because we don't see them?
No I just meant an infinite ray can't be contained inside a sphere, so rather its origin is inside the sphere. Otherwise I'm OK with this patch.
Comment Actions
Ah, I got it. Yes, you're absolutely right. The origin is inside the sphere, not the ray.
| source/maths/BoundingSphere.cpp | ||
|---|---|---|
| 49 | Actually the whole function can be written CVector3D v = m_Center - origin; float pcLen = dir.Dot(v); if (pcLen <= 0) v = (dir * pcLen) - v; return v.LengthSquared() <= m_Radius * m_Radius; | |
| source/maths/BoundingSphere.cpp | ||
|---|---|---|
| 49 | Yes, you're right, thank you! | |
| source/maths/BoundingSphere.cpp | ||
|---|---|---|
| 49 | Might want to make the 0 a 0.f as well :) | |
| source/maths/BoundingSphere.h | ||
|---|---|---|
| 50 | In fact, in Vector geometry “direction” implies a unit vector too I suppose. | |
| source/maths/BoundingSphere.h | ||
|---|---|---|
| 50 |
Yes, it does. I just remind it, for someone who isn't sure :) | |
Comment Actions
Maybe you should plan changes @vladislavbelov, unless of course it is ready to commit.