-
Notifications
You must be signed in to change notification settings - Fork 3.7k
2d picking fixes #13098
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
2d picking fixes #13098
Changes from all commits
4d69440
7d725f3
f2d476b
ba86e86
e6d62a0
6e20e37
745fe8b
3550cbf
467605c
8b877ba
983ea47
0caeca8
77b5a2f
34a08a1
f7b7e43
93c07c7
cf325cc
aa58f4c
94b259d
66ceaf5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,7 @@ function QuadtreePrimitive(options) { | |
| const tilingScheme = this._tileProvider.tilingScheme; | ||
| const ellipsoid = tilingScheme.ellipsoid; | ||
|
|
||
| this._tilesRenderedThisFrame = new Set(); // collect all tiles selected to render (useful when multiple render calls are made in a single frame (as in 2D mode)) | ||
| this._tilesToRender = []; | ||
| this._tileLoadQueueHigh = []; // high priority tiles are preventing refinement | ||
| this._tileLoadQueueMedium = []; // medium priority tiles are being rendered | ||
|
|
@@ -256,9 +257,9 @@ QuadtreePrimitive.prototype.forEachLoadedTile = function (tileFunction) { | |
| * function is passed a reference to the tile as its only parameter. | ||
| */ | ||
| QuadtreePrimitive.prototype.forEachRenderedTile = function (tileFunction) { | ||
| const tilesRendered = this._tilesToRender; | ||
| for (let i = 0, len = tilesRendered.length; i < len; ++i) { | ||
| tileFunction(tilesRendered[i]); | ||
| const tilesRendered = this._tilesRenderedThisFrame; | ||
| for (const tile of tilesRendered) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above, how many tiles are in this list? If it's a "small" number, then
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another instance where I don't think it's a hot path for performance, but it's an easy change to make, so might as well change it. |
||
| tileFunction(tile); | ||
| } | ||
| }; | ||
|
|
||
|
|
@@ -347,6 +348,7 @@ QuadtreePrimitive.prototype.beginFrame = function (frameState) { | |
| } | ||
|
|
||
| this._tileReplacementQueue.markStartOfRenderFrame(); | ||
| this._tilesRenderedThisFrame.clear(); | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -1303,6 +1305,7 @@ function screenSpaceError2D(primitive, frameState, tile) { | |
|
|
||
| function addTileToRenderList(primitive, tile) { | ||
| primitive._tilesToRender.push(tile); | ||
| primitive._tilesRenderedThisFrame.add(tile); | ||
| } | ||
|
|
||
| function processTileLoadQueue(primitive, frameState) { | ||
|
|
||
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 is going to use a spherical approximation and not the true ellipsoid circumference. Is that adequate for the use case here, or should the true circumference be used?
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 point.... I need to think about this. It does seem to "work" in all my testing but is it correct?
Perhaps the reason it's valid (at least in the testing I've done) is because our two common projections (mercator and web geographic) are cylindrical projection, where this math is exact.
It might not be valid for other projections.
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.
Well, conveniently those are the only two 2D projections we support. 😄 So maybe good enough, and just worth a comment explaining why.
If in doubt, using a root mean square approximation should be accurate enough for most real world applications since the radii are generally close in value. But that could be overkill given the above.
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.
Yeah the issue is that
MapProjectionis an interface technically, so people could in theory extend it with their own projection implementations that may not be cylindrical.In practice this hasn't happened in 15+ years so... (there are plenty of places that assume our two projections are the ONLY projections, for better or for worse).
Will leave a comment though.
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 15 years was the limit: #13149
I could imagine that people would have liked to use other projections, but accepted that this is simply not possible, despite
MapProjectionbeing an interface.