-
Notifications
You must be signed in to change notification settings - Fork 3.7k
fix(billboards): Rasterize SVG content at billboard resolution #13020
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
Conversation
|
Thank you for the pull request, @donmccurdy! Welcome to the Cesium community! In order for us to review your PR, please complete the following steps:
Review Pull Request Guidelines to make sure your PR gets accepted quickly. |
|
My two cents:
Separately, I wonder if we have considered an SDF-approach for SVG textures. As described in this video. I think we use this technique (or something like it) for text-rendering, already. In the video, it's applied to PNGs, so I'm not actually sure if it can work with SVGs, or if it can work at runtime (they show it as a preprocess step, but it looks like it can be done dynamically)... anyway just thoughts I'm thinking. Overall, this PR is realistically going to get us most of the way there. |
|
I can confirm that @donmccurdy is now covered by a CLA. |
|
Thanks @ggetz and @mzschwartz5!
Agreed, though not sure on the best followup approach yet. Another idea might be to use the existing image cache, but include image size as part of the cache key. Perhaps instead of the exact image size, that could be snapped to some nearest preset size (16, 32, 64, 128, 256) so that we're allocating "at most" 256x256x133% resolution per SVG, regardless of how many unique sizes are created in the scene.
Hm, hard to say. It could go the other way, if the browser happened to be allocating a large resolution for an SVG only displayed at 10px size. On the bright side I think there's a natural upper bound on how large the billboard's display size will be, especially if we clamp it as mentioned above. The SVG-to-SDF idea is a good one I think! I haven't seen that done yet. Any idea how common SVG billboards are, compared to images? I'm hoping to take a closer look at the SDF-based label rendering in a future PR too, but I think the current font-to-SDF rendering dependency only supports fonts. |
|
I've made a small change so that |
bfa7fe9 to
5ecf10e
Compare
|
Ready for review!
I briefly implemented this, and the issue I ran into was that the ID (used as the cache key) is also returned from
... which makes me nervous to append anything to the user-provided URL. I'd been attempting to create an ID like Hoping for a safe incremental improvement, I don't think the billboard texture uses mipmaps yet (could be wrong, but didn't find the code that would generate them?), but that seems like a very promising future improvement for many use cases, not just SVGs. So perhaps let's keep an approach for this PR where 1 URL = 1 cache entry. For now N>1 SVG billboards are scaled according to the first/cached texture, but that can be improved later, hopefully with mipmaps, as @mzschwartz5 suggested from the beginning. :) |
mzschwartz5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to merge (+/- some minor style comments you can choose to apply. I'll wait to merge until I hear from you on those).
Based on your comments, I think a follow-up using mipmaps snapping to fixed increments is a solid approach (not too complex, good value solution).
36a4673 to
3198066
Compare
3198066 to
cfce282
Compare
|
Thanks @mzschwartz5! I think all comments are addressed, ready to merge any time. |
|
Merging! Nice work 🚀 |
Description
This PR is a draft for discussion, proposing two related changes when rendering SVG content in billboards:
These changes are intended to improve visual quality for SVG content in billboards. Because SVGs are scalable, the internal viewBox and width/height are not necessarily the size the user wants to display, and if the user has added a billboard at 50px x 50px size, the billboard will appear pixelated if the (somewhat arbitrary) internal size of the SVG is different. Either of these changes independently provides a workaround for the issue. With (1) the SVG is automatically rasterized at billboard resolution, and with (2) the user can construct the HTMLImageElement themselves, resize it, and pass that to the billboard constructor.
It's not obvious to me that these changes, especially (1), are the 'right' approach ... is it OK for the billboard size to determine the resolution of the SVG in the texture atlas?
What if billboard size were in meters?(fixed) But it seemed like a good place to start discussion.Test code:
Before:

After:

Remaining work and known issues:
Issue number and link
Related:
This PR doesn't fully fix the issue, but improves quality when the billboard and SVG sizes don't match. More fixes (probably related to pixel alignment and anti-aliasing, as discussed in the issue) would be good followups in another PR.
Testing plan
Large billboard = high-resolution SVG texture: Given a larger billboard (100px x 100px), the billboard texture should be scaled up so that the SVG appears crisp.
Billboard size in meters = default texture size: If billboard enables sizeInMeters, we cannot use width/height as indicators of billboard texture size, and the default browser-detected size will be used instead.
Multiple billboards, mixed sizes: Given multiple billboards of different sizes reusing the same SVG, each billboard must (at least) render at the expected on-screen size. Optimal texture resolution is not guaranteed - the first billboard texture for a given URL is cached — but this behavior may change in the future.
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change