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
feat: Add
Ray2
class to be used in raytracing/casting #1788feat: Add
Ray2
class to be used in raytracing/casting #1788Changes from 17 commits
1d88930
84a481d
293b1ae
5f7530a
fafb1a8
7db6187
bbe5273
a479883
7da3094
4943bfa
2d8ae6e
ef9e271
d592ed4
8b9c710
17222bd
4674efb
4628fb8
ee43175
9b00711
1d31cb2
1304a28
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.
I wonder if we can handle this edge case better? For example, adding
if (dot == 0) { ... }
.Or was this a deliberate choice?
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.
That would be nice, what should we return in that case though, would
t1
still be a valid point on the line segment?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.
Ah, obviously it can't return
t1
since that would be having a division by zero.I'll try around a bit, if you have a good idea of how it can be done I'm all ears.
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.
What do you think about the solution that I pushed now?
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, maybe it makes sense that it doesn't collide with parallel rays, because it will always collide with one of the line segments connecting to it anyways?
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 guess it makes sense, esp since the intersection in this case is not a single point but rather a segment, so returning a single point would be wrong.
But we should probably explain this in a comment, because I suspect we'll soon forget what the reason was.
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 idea, I added that in #1785
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.
Sorry I'm on the phone rn, but I'd do smth like this here:
This ensures that we return a point closest to Ray's origin, and correctly handle the case when ray and segment are on parallel lines.
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, maybe it makes sense that it doesn't collide with parallel rays, because it will always collide with one of the line segments connecting to it anyways? I'll remove it again for now and then we can re-add it again later if we choose to.
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.
This method should probably need to be tested too. In particular, I'm not sure whether the sign of
t1
is correct.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.
Added some simple tests for it, please have a look to see whether they are testing what you were wondering about.