Skip to content
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.

Sign up for GitHub

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

Fix invalid tile key issues when swapping rasters when region picker active #105

Merged
merged 9 commits into from
Feb 15, 2024
Merged

Fix invalid tile key issues when swapping rasters when region picker active #105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
757f8b1
check if the tile exists before accessing
Shane98c Feb 14, 2024
7ce2389
cleaner
Shane98c Feb 14, 2024
20cae37
revert tile check
Shane98c Feb 14, 2024
3d3ea9f
update usecontrols - default values, rm unfired 'load' listener
Shane98c Feb 14, 2024
a456b0d
cleanup line break
Shane98c Feb 14, 2024
6d4c2cb
setup listener once
Shane98c Feb 15, 2024
d34970f
update level after initialization
Shane98c Feb 15, 2024
15279ec
revert useEffect deps change
Shane98c Feb 15, 2024
d4e264a
move level update to initialization step
Shane98c Feb 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/tiles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ export const createTiles = (regl, opts) => {
}) => {
if (setMetadata) setMetadata(metadata)
this.maxZoom = maxZoom
this.level = zoomToLevel(this.zoom, maxZoom)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sooo much cleaner than the callback!!

Question: in the case that triggered the race condition where queryRegion runs before updateCamera (rendering a new Raster in an existing Map with an already active region), will the tiles from the max zoom level always be fetched and queried regardless of what level the tiles actually end up rendering at?

For example, if the map is at zoom ~0 but maxZoom === 1, will the 1-level tiles be fetched by queryRegion while the 0-level tile is fetched for updateCamera? While not a huge deal in this example, this could potentially be quite costly for pyramids with high maxZooms.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm if i'm following correctly I think the zoomToLevel function returns the lesser of zoom vs maxzoom, which is what's used by queryRegion, so my understanding is that it should fetch the data for the relevant current zoom of the map, not the maxZoom. This works because this.zoom has been set by updateCamera previously. There might be a possible edge case where if you're swapping out rasters while actively zooming something could get funky - but I don't think this fix makes that situation worse than it would be normally. If zoom is for some reason undefined, I think zoomToLevels will return zero, so I don't see a way that we'd be fetching data for higher levels than needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works because this.zoom has been set by updateCamera previously.

Ah! I thought that we were hitting this case because updateCamera hadn't finished by the time queryRegion ran but it's actually because updateCamera can run before initialized resolves, right? And because of this we saw the real value of this.zoom but an unexpected value for this.level in queryRegion?

Then this all feels great, especially with the assurance that

If zoom is for some reason undefined, I think zoomToLevels will return zero

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I think that's right! we had a real zoom but maxZoom was undefined because we hadn't finished initializing yet so we were getting a level set based on zoom, with no consideration for maxZoom

So another potential spot issues could arise is if there are places in updateCamera where level not respecting maxZoom could cause issues. It looks like level is used a few times throughout. and some bad tile keys are created, but I think the check for this.loaders[level] prevents anything from going further than that.

const position = getPositions(tileSize, mode)
this.position = regl.buffer(position)
this.size = tileSize
Expand Down
10 changes: 5 additions & 5 deletions src/use-controls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import { flushSync } from 'react-dom'
import { useMapbox } from './mapbox'

export const useControls = () => {
const [zoom, setZoom] = useState()
const [center, setCenter] = useState()
const { map } = useMapbox()
const [zoom, setZoom] = useState(map.getZoom())
const [center, setCenter] = useState(map.getCenter())

const updateControlsSync = useCallback(() => {
flushSync(() => {
Expand All @@ -15,10 +15,10 @@ export const useControls = () => {
}, [])

useEffect(() => {
setZoom(map.getZoom())
setCenter(map.getCenter())
map.on('load', updateControlsSync)
map.on('move', updateControlsSync)
return () => {
map.off('move', updateControlsSync)
}
}, [map])

return { center: center, zoom: zoom }
Expand Down
Loading