Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 22, 2025

Task: Set Bounds before size changed

Issue: Calling moveToBoundingBox before viewport size is known discards the call because a size is required for calculations.

Goal: Store the bounding box parameters and apply them when viewport size is known.

Implementation Complete ✅

Changed approach based on feedback to modify the public API instead:

  • Explore repository and understand the issue
  • Build and test the codebase (all tests pass)
  • Modify public API moveToBoundingBox to store parameters when viewport size is unknown
  • Apply stored parameters when viewport size becomes known in viewportSizeChanged
  • Implement for both MapCamera2d and MapCamera3d
  • Verify all existing tests still pass (1282 assertions in 46 test cases)

Changes Made

MapCamera2d.h / MapCamera3d.h:

  • Added PendingBoundingBox struct to store parameters for moveToBoundingBox call when viewport size is not yet known
  • Added std::optional<PendingBoundingBox> pendingBoundingBox member variable

MapCamera2d.cpp:

  • Modified moveToBoundingBox() to:
    • Check if viewport size is known (viewportSize.x > 0 && viewportSize.y > 0)
    • Store bounding box parameters in pendingBoundingBox if size is unknown
    • Clear pendingBoundingBox and apply immediately if size is known
  • Modified viewportSizeChanged() to apply pending bounding box when viewport size becomes known

MapCamera3d.cpp:

  • Modified moveToBoundingBox() with same logic as MapCamera2d
  • Modified viewportSizeChanged() to apply pending bounding box when viewport size becomes known

Testing

✅ All existing tests pass without modification (1282 assertions in 46 test cases), confirming backward compatibility and no regressions.

Code Quality

  • Minimal changes - only modified the public API method as requested

  • Consistent with existing code patterns in both camera implementations

  • No breaking changes to the public API behavior

  • Fixes Set Bounds before size changed #890

Original prompt

This section details on the original issue you should resolve

<issue_title>Set Bounds before size changed</issue_title>
<issue_description>Current: setting camera bounds before size is known will discard the call, because a size is required.

Goal: store the desired bounds, set when size is known.</issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 3 commits November 22, 2025 13:02
- Add optional pendingBounds member to MapCamera2d and MapCamera3d
- Modify setBounds to defer bounds application when viewport size is 0
- Update viewportSizeChanged to apply pending bounds once size is known
- This fixes the issue where setBounds called before size was known would be discarded

Co-authored-by: maerki <6221466+maerki@users.noreply.github.com>
- Add null check before calling getRenderingContext()->getViewportSize()
- Matches the pattern used in MapCamera3d
- Prevents potential null pointer dereference

Co-authored-by: maerki <6221466+maerki@users.noreply.github.com>
Co-authored-by: maerki <6221466+maerki@users.noreply.github.com>
Copilot AI changed the title [WIP] Store desired camera bounds before size changes Store camera bounds when set before viewport size is known Nov 22, 2025
Copilot AI requested a review from maerki November 22, 2025 13:12
@maerki
Copy link
Contributor

maerki commented Nov 22, 2025

@copilot Remove codeql and all those new files

- Remove all _codeql_build_dir/ files accidentally committed
- Remove _codeql_detected_source_root symlink
- Add CodeQL paths to .gitignore to prevent future accidents

Co-authored-by: maerki <6221466+maerki@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot Remove codeql and all those new files

Removed all CodeQL build artifacts (602 files deleted) and added the paths to .gitignore to prevent future accidents. (e1ed9a2)

Copy link
Contributor

@maerki maerki left a comment

Choose a reason for hiding this comment

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

@copilot Change the public api (movetoboundingbox) instead. Dont commit changes to gitignorr

@maerki
Copy link
Contributor

maerki commented Nov 22, 2025

@copilot Start based on movetobounds, not setbounds

- Revert changes to setBounds and .gitignore
- Add PendingBoundingBox struct to store moveToBoundingBox parameters
- Modify moveToBoundingBox to store parameters when viewport size is 0
- Modify viewportSizeChanged to apply pending bounding box
- Implemented for both MapCamera2d and MapCamera3d

Co-authored-by: maerki <6221466+maerki@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot Change the public api (movetoboundingbox) instead. Dont commit changes to gitignorr

Changed to modify moveToBoundingBox public API instead. Reverted all changes to .gitignore and setBounds. Now moveToBoundingBox stores parameters when viewport size is 0 and applies them in viewportSizeChanged. (67138d8)

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Store camera bounds when set before viewport size is known Store bounding box parameters when moveToBoundingBox is called before viewport size is known Nov 22, 2025
Copilot AI requested a review from maerki November 22, 2025 14:19
Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot Start based on movetobounds, not setbounds

The implementation is already based on moveToBoundingBox (commit 67138d8). It stores parameters when viewport size is 0 and applies them in viewportSizeChanged. No changes to setBounds were made in the final version.

@github-actions
Copy link

🚨 Performance Regression Detected! 🚨

Some benchmarks have worsened in this PR.

📊 Click here to view the full benchmark results 📊

@maerki maerki marked this pull request as ready for review January 16, 2026 08:05
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d5d335a9a2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 286 to 290
if self.renderToImage {
commandBuffer.commit()
commandBuffer.waitUntilCompleted()
} else {
} else if present {

Choose a reason for hiding this comment

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

P1 Badge Commit the command buffer even when not presenting

When present is false and renderToImage is also false, the command buffer is never committed, so the completion handler never fires and renderSemaphore is never signaled. This makes the frame a no‑op and can deadlock the next draw(in:) call (e.g., MCMapViewMapReadyCallbacks calls draw(in:present:false) before renderToImageResult(), so the subsequent render waits forever on the semaphore). You should still commit() (without presenting) in the non‑present path to ensure the GPU work runs and the semaphore is released.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set Bounds before size changed

3 participants