fix: prioritize tracked world map tile requests#5747
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
Implemented the request-ordering change so the tracked/searched map block is requested before surrounding visible blocks, while preserving the existing 64-region request cap.\n\nLocal validation in this runner:\n- git diff --check\n- targeted source sanity checks\n\nI could not run a full build here because this environment does not have cmake or scons installed. |
|
recheck |
|
Hi! Just checking in — please let me know if any changes are needed. Happy to update the PR. Thanks! |
|
\n I have read the CLA Document and I hereby sign the CLA\n |
|
recheck |
|
Hi! Just checking in \u2014 please let me know if any changes are needed. Happy to update the PR. Thanks! \ud83d\ude42 |
| // We use fixed sized blocks for ease of storage and lookup, | ||
| // but the requests can be of variable size of up to | ||
| // MAX_REQUEST_REGIONS. | ||
| S32 block_x0 = x0 / MAP_BLOCK_SIZE; |
There was a problem hiding this comment.
Please don't remove comments. This logic is complicated to parse without having at least some idea of what to expect.
There was a problem hiding this comment.
Pull request overview
This PR improves perceived responsiveness of the in-viewer World Map by prioritizing the map-block request containing the currently tracked (searched) location when that block is within the visible region range, while preserving the existing per-request 64-region server limit. It also adds a regression unit test to ensure the tracked block request is issued first and that batching still respects the request-size constraints.
Changes:
- Request the tracked map block before surrounding blocks when it falls inside the current
updateRegions()rectangle. - Ensure subsequent batched rectangle requests do not include the tracked block and continue to cap each request at 64 regions.
- Add unit coverage verifying “tracked-block-first” behavior and the existing batching/splitting behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
indra/newview/llworldmap.cpp |
Updates LLWorldMap::updateRegions() to issue a prioritized request for the tracked block (when visible) and avoid including it in later grouped requests. |
indra/newview/tests/llworldmap_test.cpp |
Adds a regression test asserting the tracked block is requested first and that each emitted request stays within the 64-region limit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LLVector3d tracked_pos(9.0 * REGION_WIDTH_METERS, 9.0 * REGION_WIDTH_METERS, 0.0); | ||
| mWorld->setTracking(tracked_pos); | ||
| mWorld->updateRegions(0, 0, 15, 15); | ||
| ensure("updateRegions tracked block: expected 5 requests", gMapBlockRequests.size() == 5); | ||
| ensure("updateRegions tracked block: first request min_x", gMapBlockRequests[0].min_x == 8); |
| << ") [" << REGIONS_PER_BLOCK << " regions]" << LL_ENDL; | ||
|
|
||
| LLWorldMapMessage::getInstance()->sendMapBlockRequest(min_x, min_y, max_x, max_y); | ||
| mMapBlockLoaded[tracked_offset] = true; |
There was a problem hiding this comment.
- Does this cover all 3 types of tracking, or just search search? There is no reason to prioritize just the search. Ex: If user triggers a trackLandmark, user wants that loaded first.
- LLFloaterWorldMap::trackLocation already requests the tile, I'm not sure if there is a reason to request it again here. But it does not set mMapBlockLoaded for some reason, might be a bug there. Nor are other types of tracking doing the same
After checking the code I'm not sure if this ticket actually needs fixing or if #5558 fixed it and it just needs to go through QA first to make sure issue is gone.
| const S32 check_block_x = block_x + request_width; | ||
| const S32 check_offset = check_block_x | (block_y * MAP_BLOCK_RES); | ||
| if (mMapBlockLoaded[check_offset] || | ||
| (tracked_block_visible && check_block_x == tracked_block_x && block_y == tracked_block_y)) |
There was a problem hiding this comment.
What's the point of checking this? Wouldn't mMapBlockLoaded[offset] already be true?
Summary
Testing
git diff --checkcmakeandsconsare unavailable)Fixes #5735