-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 merged raycastFirst
and raycastAll
into raycast
#5039
Open
MushAsterion
wants to merge
100
commits into
playcanvas:main
Choose a base branch
from
MushAsterion:shapecasts
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 14 commits
Commits
Show all changes
100 commits
Select commit
Hold shift + click to select a range
ac625fc
Added shape casts
MushAsterion 4fd6099
Fixed ESLint
MushAsterion c5f8fc8
Fixed halfExtends into halfExtents
MushAsterion 36d500a
Added shapecast shape destroying
MushAsterion 624c8da
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 0e51d63
Reduced debug assets into one
MushAsterion 2ace6a3
Removed variable type change from shapeCasts
MushAsterion 2b1e039
Fixed JSDoc for optional parameters
MushAsterion 0cdc17a
Added default position and rotation for _shapecast
MushAsterion 052e8f8
Removed shapecast body world addition
MushAsterion 7eff8db
Shape casts now return Entity array
MushAsterion 5de3fef
Removed unused variables
MushAsterion 9133182
Added Entity import
MushAsterion 1e4415e
Fixed Entity JSDoc
MushAsterion 53cb43f
Updated shape.type eval to switch
MushAsterion b4f005b
Fixed docs
MushAsterion 502994c
Fix shape on boxCast
MushAsterion c8adb46
Added HitResult to shapecast
MushAsterion fd907d5
Fixed inconsistent documentation
MushAsterion 9566b60
Fixed ESLint
MushAsterion 6c2d55b
Changed naming from cast to test
MushAsterion 9027bf3
Added Quat rotation for shape tests
MushAsterion c294abb
Fixed ESLint
MushAsterion 845fb0a
Added sphereCast and boxCast
MushAsterion 34d83b3
Fixed typo
MushAsterion a448e62
Merge branch 'playcanvas:main' into shapecasts
MushAsterion fcb70ea
Fixed sphereCast length
MushAsterion 6213685
Added vector for shapecast position
MushAsterion a444069
Fixed boxCast shape halfExtents
MushAsterion 3ae5522
Fixed typo
MushAsterion dc69842
Renamed boxCastAll and sphereCastAll
MushAsterion 2d8149a
Fixed sphereCast length
MushAsterion 8134d3f
Removed boxCastAll
MushAsterion 87451da
Removed shapetest body deactivation
MushAsterion d66eaaf
Merge branch 'playcanvas:main' into shapecasts
MushAsterion bf5c384
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 73b10a7
Rename shape casts into shapeCastAll
MushAsterion 2fc5fa8
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 3df7a28
Merge branch 'playcanvas:main' into shapecasts
MushAsterion d6d4539
Changed RaycastResult into HitResult
MushAsterion a9ea632
Changed RaycastResult into HitResult
MushAsterion db8274b
Changed RaycastResult into HitResult
MushAsterion faf1f00
Deprecated RaycastResult
MushAsterion 6474880
Merge branch 'playcanvas:main' into shapecasts
MushAsterion e084be1
Added shapeCastFirst functions
MushAsterion c344241
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 2759da4
Merge branch 'playcanvas:main' into shapecasts
MushAsterion cf21ed8
Changed T and C to lowercase from Test and Cast
MushAsterion f5d91b6
Reverted previous commit
MushAsterion 8ca124e
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 00d2d67
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 1ccc26a
Reduced code length
MushAsterion a4a8305
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 54518b1
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 73b751c
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 77b3ed0
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 956fd3e
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 377f5ae
Added support for `shapeTestFirst`
MushAsterion 383e9ad
Resolve conflicts from source
MushAsterion 390bac6
Merge remote-tracking branch 'upstream/main' into shapecasts
MushAsterion e5c7a09
Match latest features from raycasting
MushAsterion 9802d4c
Added sorting documentation.
MushAsterion 6bd474b
Merge remote-tracking branch 'upstream/main' into shapecasts
MushAsterion ea47f72
Merge remote-tracking branch 'upstream/main' into shapecasts
MushAsterion 969ab5b
Added filtering to shape testing and casting
MushAsterion 290a35d
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 7c1b2c1
Merge remote-tracking branch 'upstream/main' into shapecasts
MushAsterion 20f6377
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 6fc799a
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 4c70791
Fixed tags filtering issue
MushAsterion e6a47a3
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 230e1fa
Merge branch 'playcanvas:main' into shapecasts
MushAsterion f0ab87b
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 79a67ee
Merge branch 'playcanvas:main' into shapecasts
MushAsterion ecf1583
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 74f509a
Merge branch 'playcanvas:main' into shapecasts
MushAsterion f23587a
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 3c3fa50
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 13a7c28
Merge branch 'playcanvas:main' into shapecasts
MushAsterion fa155c4
Merge remote-tracking branch 'upstream/main' into shapecasts
MushAsterion f373ac7
Merge branch 'playcanvas:main' into shapecasts
MushAsterion f955f64
Merge branch 'playcanvas:main' into shapecasts
MushAsterion ff3906b
Merge branch 'playcanvas:main' into shapecasts
MushAsterion b96694a
Merge branch 'playcanvas:main' into shapecasts
MushAsterion ece932e
Merge branch 'playcanvas:main' into shapecasts
MushAsterion d442376
Merge remote-tracking branch 'upstream/main' into shapecasts
MushAsterion a1a5473
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 8c7a842
Merge remote-tracking branch 'upstream/main' into shapecasts
MushAsterion 2b2b643
Fixed trailing space
MushAsterion 654794e
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 02e33bf
Merge branch 'playcanvas:main' into shapecasts
MushAsterion 7bae7dc
Merge branch 'main' into shapecasts
MushAsterion c5e35d6
Reduced public API functions
MushAsterion ba29b58
Used Object assign instead of ...
MushAsterion 8c67c9f
Fixed documentation disclaimers
MushAsterion b3f213d
Changed raycastFirst/raycastAll occurrences
MushAsterion 7a642a8
Removed extra Ammo.destroy
MushAsterion 972c1f7
Fixed documentation and line width
MushAsterion ec7aa86
Merge branch 'main' into shapecasts
MushAsterion bae27b7
Merge branch 'main' into shapecasts
MushAsterion File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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 🙃