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.

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

Add Undefined mercatorOrigin Check to computeMercatorFractionToDb #7269

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

andremig-bentley
Copy link
Contributor

@andremig-bentley andremig-bentley commented Oct 15, 2024

This PR is to address this issue: https://github.com/iTwin/itwinjs-backlog/issues/1243

Currently, there is an edge case where if new BackgroundMapGeometry is created with project extents centered at (0,0,0) and an ecefLocation.origin of (0,0,0), the creation of the BackgroundMapGeometry will fail. When the point (0,0,0) is converted from ECEF to a Cartographic using Cartographic.fromEcef, the function will return undefined and cause an error to occur on the creation of BackgroundMapGeometry in the above scenario.

To fix this, ecefToPixelFraction has been changed to now also return undefined if fromEcef returns undefined, and a check has been added to computeMercatorFractionToDb to return an identity transform if mercatorOrigin, mercatorX, or mercatorY is undefined due to the change in ecefToPixelFraction. A test has also been added to BackgroundMapGeometry.test.ts which tests the scenario where the project extents are centered at (0,0,0) and the ecefLocation.origin is (0,0,0) and confirms the succesful creation of BackgroundMapGeometry.

@pmconne
Copy link
Member

pmconne commented Oct 16, 2024

Did you do the following?

  1. Obtain or create iModel exhibiting the bug
  2. Confirm that without your fix, opening the iModel in display-test-app with map display enabled produces the same error .
  3. Repeat 2 with your fix.
  4. Confirm map displays correctly as you navigate the view.

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.

2 participants