Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added support for
shapeCast
and mergedraycastFirst
andraycastAll
intoraycast
#5039base: main
Are you sure you want to change the base?
Added support for
shapeCast
and mergedraycastFirst
andraycastAll
intoraycast
#5039Changes from 3 commits
ac625fc
4fd6099
c5f8fc8
36d500a
624c8da
0e51d63
2ace6a3
2b1e039
0cdc17a
052e8f8
7eff8db
5de3fef
9133182
1e4415e
53cb43f
b4f005b
502994c
c8adb46
fd907d5
9566b60
6c2d55b
9027bf3
c294abb
845fb0a
34d83b3
a448e62
fcb70ea
6213685
a444069
3ae5522
dc69842
2d8149a
8134d3f
87451da
d66eaaf
bf5c384
73b10a7
2fc5fa8
3df7a28
d6d4539
a9ea632
db8274b
faf1f00
6474880
e084be1
c344241
2759da4
cf21ed8
f5d91b6
8ca124e
00d2d67
1ccc26a
a4a8305
54518b1
73b751c
77b3ed0
956fd3e
377f5ae
383e9ad
390bac6
e5c7a09
9802d4c
6bd474b
ea47f72
969ab5b
290a35d
7c1b2c1
20f6377
6fc799a
4c70791
e6a47a3
230e1fa
f0ab87b
79a67ee
ecf1583
74f509a
f23587a
3c3fa50
13a7c28
fa155c4
f373ac7
f955f64
ff3906b
b96694a
ece932e
d442376
a1a5473
8c7a842
2b2b643
654794e
02e33bf
7bae7dc
c5e35d6
ba29b58
8c67c9f
b3f213d
7a642a8
972c1f7
ec7aa86
bae27b7
3ef58e5
8aecf7a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a utility function that basically calls the shape-specific functions. For the purpose of maintaining a nice and accessible API, should we:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantage of "all-in-one" function is that you can actually use a JSON format for all the shape config meaning it can easily be assigned from a script attribute to edit it within the editor directly and avoid having to be only code-based. In another hand the shape-specific function allow users used to other engines to find the function easily (especially
sphereCast
which is really common).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's a great point. OK, fair enough. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to unresolve this discussion and talk about it some more, @MushAsterion. I do still feel that this PR introduces a lot of additions to the API. 18 methods at the moment. And if
shapeCastAll
flavors get added, it takes it to 24. Plus the two existing raycasting functions, that'd be 26 functions for testing and casting. I think that's just too much. We could just have:The all versus first could just be an option (maybe defaulting to first).
It just feels to me that the JSON approach is interesting, but it's very niche and, in reality, the vast majority of developers won't call the API like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could potentially also drop
shapeTest
if start and end pos/rot ofshapeCast
where the same too maybe?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shapeTestFirst
is doing more work due to how Bullet/Ammo work and completely different from raycasting as raycasting is only a question of point while the closest point in a shape's position is not always the first to be detected since it depends on how the physics engine perform the test.Once again the exposure of First/All methods is to match with what the engine has already implemented. I'm an external contributor so I don't want to step or change core project features. I personally find it handy for beginners to have the support for sure but as @willeastcott mentioned it's a very long list of new public methods which might also bring more confusion. I think it's a good solution to merge as much as possible to reduce public methods but I'm concerned about how easy it would be for people that are just looking to make a quick project without trying to get a deep understanding of the engine's API... I'll surely update to fit whatever they think is best anyway, the core features of the PR are already done and updating them is not a big deal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! Many useful features are being added. Although, I do agree that the API is too verbose. We can simplify those, and move most of the customization stuff to options.
My vote goes for dropping -all/-first from the method name, and use an option setting.
shapeTestFirst
seems a bit superfluous. We can just return everything and let user decide what to do with the results. If future Ammo adds a feature to allow collecting only the first intersection (which sounds strange to me), we can add it via an option. Shape and point collision tests are usually useful for getting all colliders they intersect, otherwise a sweep test is used.About shape target rotation during sweep. Personally, I've never met a need to use it. Maybe it is useful in robotics, where Bullet engine is popular? No idea. Most of the other other physics engines don't allow it (Rapier, PhysX, Jolt). As such, I would actually move it to options as well.
Whatever the names are, but signatures could look something like:
If
dir
is a zero vector, we do shape test. Otherwise it is a non-normalized cast direction (its normal for direction, its magnitude as cast distance). Shape properties can be part of the options, e.g. radius for radius-based shapes. We could also use the end position instead and check if it is same as start position, as Will suggested. Perhaps would be a better match to raycast then.About destroying a shape used in sweep. If a shape is not auto-destroyed (e.g. via some option), then there should be a method to destroy it later.Never mind, it is for a user created one, which he is in control of.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand the point of changing
endPosition
intodir
whileraycast
has aend
param, is there any particular reason to make it different?Something like
Where options could have something like
Would be more fitting with the suggested
raycast
signature.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, something like that. I think starting rotation is probably not in options, as it is often used for non-spheres. Target rotation can be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright so based on feedback I just rewrote the PR. Let me know if it better fits now 🙃