Improve clipping polygon performance #12650
Conversation
|
Thank you for the pull request, @javagl! ✅ We can confirm we have a CLA on file for you. |
|
@ggetz can this be reviewed to be merged ? |
|
@javagl Thanks a lot for this fix on the performance of the Clipping Polygon tool ! We tested it and it's way more efficient than the previous version ! |
| function copyArrayCartesian3(input) { | ||
| if (!defined(input)) { | ||
| return undefined; | ||
| } | ||
| const n = input.length; | ||
| const output = Array(n); | ||
| for (let i = 0; i < n; i++) { | ||
| output[i] = Cartesian3.clone(input[i]); | ||
| } | ||
| return output; | ||
| } |
There was a problem hiding this comment.
This seems like something that might be useful in other contexts. Does it belong in the Cartesian3 class itself?
There was a problem hiding this comment.
Same below with the equalsArrayCartesian3 function
There was a problem hiding this comment.
I'm pretty sure that similar things are done at many places, and this and similar trivial things are "inlined" everywhere (The size of the codebase could be reduced significantly by avoiding this sort of duplication). There's a tension between 1. trying to keep the API surface area small and 2. identifying the parts that should be accessible and actually use them (and that "using them" part is the difficulty when such fundamental building blocks are not identified and offered right from the beginning...)
I'll schedule another review over the code, and try to identify places where CartesianX arrays are copied or compared.
(BTW: If we offer this for Cartesian3, we also have to offer it for all other CartesianX classes, and maybe even for the MatrixX classes - otherwise, these classes diverge and become (too) inconsistent...)
There was a problem hiding this comment.
OK, there certainly are many places where such functions could have been useful, and could have been used, to avoid certain redundancies, but ... now the redundancies are there. And while I strongly agree that such "bulk" operations should at least be considered, I don't think that this PR is the right place and time to publicly introduce the two specific ones that happen to be required here.
I opened #12780 with further thoughts (we don't want that to stay "1.4k" for too long, eh...? 😁 )
|
PR looks good to me- just have a comment about where to put a couple functions |
Description
Cache the "rectangle" that is computed in the
ClippingPolygonclass, so that it does not have to be re-computed each frame.As mentioned in this issue, the clipping polygons had a distressingly low performance for a higher number of points. As analyzed in that issue, this was largely caused by the
computeRectanglefunction inClippingPolygon, which did re-compute the rectangle each frame.This PR caches the rectangle, and only re-computes it when the
_positionsof the polygon change. For that, it was necessary to actually also cache the positions, and compare the cached positions with the actual positions (there is no other way to detect changes in the positions).Issue number and link
Addresses #12258
Testing plan
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change