Web MapSDK #909
Web MapSDK #909
Conversation
🖼️ Image Diff Test FailureThe image comparison tests failed. Rendered images don't match the expected golden images. Investigate:
To approve the changes:
Last updated: Run #576 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cf9eb64e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| auto lineHeight = common.at("lineHeight").get<double>(); | ||
| auto base = common.at("base").get<double>(); | ||
| auto imageSize = common.at("scaleW").get<double>(); | ||
|
|
||
| const FontWrapper fontWrapper{name, lineHeight / size, base / size, Vec2D(imageSize, imageSize), size, distanceRange}; |
There was a problem hiding this comment.
In FontManifestParser::parse, the atlas height is assumed to be the same as scaleW (imageSize is reused for both dimensions). For fonts whose JSON uses a non‑square atlas (scaleW != scaleH), this makes the FontWrapper size and the subsequent texture coordinate normalization use the wrong vertical scale, which will misplace or distort glyphs at render time. This should read scaleH separately and normalize t0/t1 with that height to keep glyph UVs accurate for non‑square atlases.
Useful? React with 👍 / 👎.
| // perform the load with the mutex kept _locked_. | ||
| // -> no parallel loading of different fonts | ||
| const std::string manifestUrl = baseUrl + font.name + ".json"; | ||
| const std::string textureUrl = baseUrl + font.name + ".png"; | ||
| return loadFontFromUrls(*dataLoader, manifestUrl, textureUrl); |
There was a problem hiding this comment.
FontLoader::loadFont checks the cache but never inserts the newly loaded FontLoaderResult before returning. In the web build, TextLayer and other callers invoke loadFont repeatedly for the same font name, so this omission causes repeated network fetches and JSON parsing for every call, which can noticeably slow down rendering and increase bandwidth. After loadFontFromUrls succeeds, store the result in cache (e.g., keyed by font.name) so subsequent calls reuse it.
Useful? React with 👍 / 👎.
|
I forgot to make one change: I had to rename a parameter in the logger because it was a keyword in JS. I still have to make the corresponding change for iOS and Android |
komplexon3
force-pushed
the
feature/web-map-sdk
branch
from
888f9c1 to
1daef30
Compare
Initial version of the "Web MapSDK".
I added the patch from the exploration-repository and added checks to pick the correct shader version.
@matzf I marked bits that need to be looked at with
TO_CHECK.Note that the referenced djinni version is a feature branch.