-
Notifications
You must be signed in to change notification settings - Fork 63
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Another attempt at a proper intersection Trace in clipTo
- Loading branch information
Showing
1 changed file
with
34 additions
and
8 deletions.
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
f4aba13
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.
For this to work I think we would have to change traces to return a list of all intersections. (Which might not be a bad idea anyway.)
f4aba13
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.
Getting the list of intersections in the Trace would make this code much simpler. I believe you that it's necessary. Right now my code finds the same intersection over and over again, which isn't good. I also need to review under what circumstances Traces give negative distances, before I write any more code dealing with Traces.
f4aba13
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 should we do with
clipTo
in the meantime:f4aba13
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.
Why do we need to do anything with
clipTo
"in the meantime"? Why not just do it right in the first place? Changing traces to return a list of intersections should not actually be that hard (...fingers crossed). Basically:Trace
Trace
to stop throwing information away (e.g. it might be as simple as replacing a call tominimum
with a call tosort
!).Semigroup
forTrace
to do a merge on sorted liststraceP
etc. usinghead
.Due to laziness, this won't even incur much performance overhead compared to what we have now: taking the
head
of some merged sorted lists only has to do a few comparisons and never has to look at most of the list contents.The reason this is necessary for implementing
clipTo
is this: imagine that a ray hits the border of a diagram in four places. Currently, we can only ever get the first and last intersections out of the trace. But if those intersections lie outside the clipping path, they don't count, and there is no way to find the other two. The key point which I think @bergey was missing is that calling trace always returns the intersection with the smallest t-value, even if that intersection is "behind you" (i.e. if the t-value is negative). This is a bit unintuitive at times (it's not how raytracers typically work) but it makes all the math work out really nicely. I should add that if we switch to returning a list, we can easily implement functions that do work in the intuitive way you would expect, simply by filtering out negative t-values.f4aba13
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.
Your right, we don't have to do anything in the meantime, but I do think before we go ahead and refactor traces we should rethink "negative" traces.
f4aba13
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.
Re: rethinking negative traces, see my edited comment above. I think switching to returning a list is exactly what we need to do to be able to handle "negative" traces well.
f4aba13
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 didn't see the last part about negative traces. I'll go ahead and start working on returning a list in the clipTo branch.
f4aba13
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.
Great! And just to record here what I just said on IRC, the reason we need to return negative t-values at all is so that traces can be translated. A translation can turn negative t-values into positive, so you need to know about all the intersections in order to be able to translate traces properly. However, if we return a list, then this can just be an implementation detail---we can make the main user-facing functions filter out negative t-values.